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

Fri Mar 18 13:17:02 PDT 2022

jdenny added a comment.

In D121727#3392944 <https://reviews.llvm.org/D121727#3392944>, @tra wrote:

>> 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?

Yes, I was referring to the `%ptxas-verify` proposal mentioned earlier.

>> 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.

I'm not seeing how `%when` or `RUNIF:` accomplishes that particular goal better than `%ptxas-verify`.  Either way, the RUN line runs when ptxas is available and quietly doesn't run when it's not available.  Am I misunderstanding?

>> 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?

I was referring to the `ptxas -c -o /dev/null` proposal mentioned earlier, but now I realize that only helps with file output.  More below.

> 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`.

So the problem is someone might check ptxas diagnostic output with FileCheck?  That seems like a bad idea even when you're sure ptxas is available given that it's an external tool whose diagnostics can change over time.  At most, it seems like tests would be written to fail for a non-zero exit status and possibly a non-empty stderr, and that should work fine when expanding to `true`.

> 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`.

In comparison to the `%when`, `RUNIF:`, `%if`, etc. proposals, is this just a new syntax proposal?  Or is there another difference to consider?

> 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.

It sounds like we're getting into more use cases now, so that makes a new construct more appealing.

>> 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?

Failed lit tests have output like the following so that, if there are many RUN lines, it's easier to tell which RUN line failed:

  $ ":" "RUN: at line 1"

You might need `LIT_OPTS=-vv` to make it useful, depending on your config.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121727