Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 30: Ignore not-supported sm_* error without --verify #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tob2
Copy link
Contributor

@tob2 tob2 commented Mar 3, 2022

Issue #30

  • nvptx-as.c (PTXAS_IGNORE_ERR): Define.
    (collect_wait, do_wait, fork_execute, main): Unless --verify
    is used, don't error when the ptxas error message contains
    PTXAS_IGNORE_ERR.

@tob2
Copy link
Contributor Author

tob2 commented Mar 11, 2022

Actually, I wonder whether besides the sm_ check, there should be also a ptx check, i.e. to check for:

Unsupported .version 6.0; current version is '4.1'

	* nvptx-as.c (PTXAS_IGNORE_ERR_1, PTXAS_IGNORE_ERR_2,
	PTXAS_IGNORE_MSG_1, PTXAS_IGNORE_MSG_2): Define.
	(stderr_msg): New.
	(collect_wait, do_wait, fork_execute, main): Unless --verify
	is used, don't error when the ptxas error message contains
	PTXAS_IGNORE_ERR_*; print ignore message with '-v'(erbose).
@tob2
Copy link
Contributor Author

tob2 commented Mar 11, 2022

Updated:

  • Also handle messages like: Unsupported .version 6.0; current version is '4.1' – to cause skipping non-forced verifications
  • Note printed with -v(erbose):
    nvptx-as: not verifying: specified .target not supported by ptxas and
    nvptx-as: not verifying: specified PTX ISA .version not supported by ptxas
  • Commit title: use [as] for better consistency with other commits.

@tschwinge
Copy link
Member

Thanks for the submission, but hrm, I don't like parsing ptxas output: we don't control this, and might not even notice when it changes. So the nvptx-as behavior may then silently change if ptxas changes. That seems fragile to me generally; too much happening "behind the scenes".


In #33, I've implemented a different take on this, based on @vries' suggestion in http://mid.mail-archive.com/20220331074045.GA29630@delia.home for a "workaround: verify using sm_35 when misa=sm_30 is specified (either implicitly or explicitly)".

@tob2
Copy link
Contributor Author

tob2 commented Apr 11, 2022

Thanks for the submission, but hrm, I don't like parsing ptxas output: we don't control this, and might not even notice when it changes. So the nvptx-as behavior may then silently change if ptxas changes. That seems fragile to me generally; too much happening "behind the scenes".

First, we already have quite some assumptions about ptxas – what it supports, about the command line arguments (--gpu-name, -O0), behaviour of --gpu-name vs. .target etc.

Secondly, regarding silently: It parses for two explicit strings — I sincerely doubt that those change the meaning and, given how long they stayed constant, I doubt that they will change in the short- or midterm. Except for handling those strings in a specific way, all other output gives still an error. Except for very odd constructed cases, I do not see any case where those fail silently. If it fails on the user side because of changes, passing -Wa,--no-verify is still a possibility (likewise to force the check, -Wa,--verify).

Thirdly, the change of behaviour is not silently: with --verbose there is now an explicit warning. (Side remark: Actually, I wonder whether the program-not found should also output some kind of "warning:" or "note:" when -v has been requested.) — In any case, we could consider invoking nvptx-as with -v from GCC to make this more prominent – at least when building GCC itself. (The user could do -Wa,-v to force this warning.)

In #33, I've implemented a different take on this, based on @vries' suggestion in http://mid.mail-archive.com/20220331074045.GA29630@delia.home for a "workaround: verify using sm_35 when misa=sm_30 is specified (either implicitly or explicitly)".

I will add a comment on PR #33 itself.

Additionally: PR #33 contrary to this PR does not solve the issue of having a too old CUDA (newer PTX ISA (and .target sm_XX) not supported), which are also real issues, which can be worked around in GCC by not verifying at all, which is also pointless.

(In any case, both this PR #31 and PR #33 make it less likely to detect some issues in the generated ptx assembly but reduce the strict version dependence on a particular CUDA version. This PR at least permits to warn about this via -v.)

@vries – thoughts on your side regarding this pull request (#31) and regarding #33?

@vries
Copy link
Contributor

vries commented Apr 12, 2022

@vries – thoughts on your side regarding this pull request (#31) and regarding #33?

About the output parsing. I see the point of Thomas about parsing ptxas output being fragile, but I think there's a usual way of dealing with this. That is, use a configure process to find out what the tool supports, and use resulting HAVE_ macros to conditionalize handling in the sources.

Then again, that does add a level of complexity that may be considered too much effort to maintain.

I think that if this verify step was the only step where we do this, we'd go to extreme lengths to make this check as precise as possible. But the fact is that when the driver loads the ptx, it does the same type of checks, makes it acceptable to do compromises.

@tob2
Copy link
Contributor Author

tob2 commented Apr 12, 2022

@vries – thoughts on your side regarding this pull request (#31) and regarding #33?

About the output parsing. I see the point of Thomas about parsing ptxas output being fragile, but I think there's a usual way of dealing with this.

I concur that it is not ideal – but making some assumptions have always to be done. Regarding:

That is, use a configure process to find out what the tool supports, and use resulting HAVE_ macros to conditionalize handling in the sources.

That's a bit unclear to me. At least in common environments, the build system (for nvptx-tools, GCC etc.) is not fully identical to the one where the code is run. In particular, I expect that nvptx-tools is compiled on a system without CUDA installed or some CUDA version and then later run with other CUDA versions, be it because of using a different computer or having later updated the version.

Thus, having the check inside nvptx-as makes more sense to me than having it in a configure script.


In the issue itself, #30, a variant of using PTX Compiler APIs instead is discussed, which might be an alternative way out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants