[PATCH] D121727: [NVPTX] Integrate ptxas to LIT tests

Fri Mar 18 11:11:30 PDT 2022

tra added a comment.

In D121727#3392558 <https://reviews.llvm.org/D121727#3392558>, @jdenny wrote:

> I'm reluctant to add a new lit construct (`%when` or `RUNIF:`) based on only one use case (or are there more I missed?)

It's a use case where we currently can not easily verify the assembly we produce for the whole back-end (NVPTX). When we add new instructions, we have to test the output manually. The end result is that we've already had a handful of bugs that were detected only after they showed up long after commit and that has non-0trivial logistical consequences. E.g. if it makes it into a compiler release, it's hard to get all the users to update their compiler even if we do make a patch release with the fix.

Making things easily testable would provide an important safeguard.

> when an existing construct (lit substitution) would work easily.

Could you elaborate? Do you mean the substitution of `%ptxas` with `true` if it's not available? Something else?

> A second or third use case might reveal more generality is needed.  I'm also slightly concerned the proposed `%when` or `RUNIF:` might generally encourage people to use it when `REQUIRES` is the right thing to do (but I understand `REQUIRES` is not right for this use case).

Those were just strawman-style proposals. I don't have particular attachment to any of them. 
My main concern was that tests should not lie to the user. One should be able to tell a test that was not run from the test that ran and succeeded.

> But do we want to go down this [ complicated ] road just yet?  Maybe it needs to bake a bit longer?  The lit substitution is simple and fits your current use case well, in my opinion.

SGTM, modulo "no-lies" concern I've outlined above.

> If you go with the lit substitution, I have a few small comments.  I like the proposal to discard its output.

Which output do you have in mind?

If we go with substitution, I think the sensible thing to do would be to discard the whole RUN line if the substitution fails. Unlike substituting `%ptxas` with `true`, it would not work with RUN likes that depend on ptxas output. E.g. `%ptxas | FileCheck`.

> That makes it harder to misuse it, and it leads people back to `REQUIRES` in some cases.  The substitution could be named to more clearly indicate it might not run: `%optional-ptxas-verify` is one possibility.  When not available, it could expand to an explicit message instead of `true` to help people understand what's happening when debugging in lit's verbose mode.

What if we introduce a pseudo-tool %if-<feature>. If the feature is available it would be substituted by an empty string and would not affect the result of the substitution of the RUN line. If the substitution fails, it would disable the RUN line and we could issue a diagnostic when run in verbose mode. It would be easy to tell what's expected to happen in the test source and, as a bonus, would allow predicating RUN lines based on multiple features.

E.g. `RUN: %if-ptxas %ptxas whatever`.

This would also be useful when we need to do some back-end specific tests in a largely generic test file. E.g. when we're testing generic LLVM intrinsics we may want to verify that they produce particular instruction for a given back-end, but we don't know whether that back-end is compiled in. Right now we have to copy/paste such tests per-back-end and use REQUIRES.

> We already use the `:` command to report `RUN` line numbers, so that could be used here.  The message might be "Skipping verification because ptxas is not available".

I'm not familiar with that. Could you give me an example of how/where this is used?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121727/new/

https://reviews.llvm.org/D121727