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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D121727#3390706 <https://reviews.llvm.org/D121727#3390706>, @mstorsjo wrote:

> I think @jdenny is code owner (or the closest thing) for lit,

Not sure I am.  :-)

> I think he'd have more input on the direction here.

Yes, I can offer an opinion.  Thanks for tagging me.

I'm reluctant to add a new lit construct (`%when` or `RUNIF:`) based on only one use case (or are there more I missed?) when an existing construct (lit substitution) would work easily.  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).

If you go with a new construct, here are some additional capabilities that occur to me:

- A block syntax would be nice so you don't have to repeat the condition across multiple RUN lines and don't have to string together commands with `&&`.  Given that, a one-line syntax might not even be worthwhile.
- Besides features, it might support lit substitutions within the condition.  Some test suites (I'm thinking of libomptarget and some of my downstream work) instantiate the same lit test and the same lit RUN lines multiple times in different sub test suites for different configurations.  If those configurations are reflected in lit substitutions, then the RUN lines could vary across instantiations to some degree based on the condition in a `%when` or `RUNIF`.
- Ultimately, I imagine this would become a general preprocessor `#if` directive for lit.  Maybe name it `%if`?
- An additional operator syntax could sometimes be useful for changing parts of a command: `RUN: FileCheck -DVAR=%if(cond, then, else)`.

But do we want to go down this 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.

If you go with the lit substitution, I have a few small comments.  I like the proposal to discard its output.  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.  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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121727