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

Fri Mar 18 14:59:59 PDT 2022

tra added a comment.

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

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

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

The difference is that replacing the commend itself with `true` still executes the RUN line.

I believe that the whole "RUN" line must be handled atomically. Either all commands are executed as specified by user, or none of them, if we can't provide any of the substitutions.
We should not magically replace a command with true, execute the RUN line and expect it not to fail for all possible commands users may put on the RUN line.
lit can control whether it runs the RUN line or not, but it has no control what the user expected that command to do.
Sure, we can provide a restricted variant of the tool substitution which would explicitly require RUN line not to depend on command's outputs, only on its exit status. 
That would be unnecessarily restrictive, IMO, but we can live with that. Still, I think executing or not whole RUN line will give us more flexibility and consistency.

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

It's just one possibility. In general, it's applicable to any output the program may produce, not just what it may print on the console. It's also not specific to ptxas either. In general it may be applicable to any optionally available external tool. ptxas just happened to be the first one where the benefit was worth the trouble implementing this patch.
Even in case of ptxas, I can see how we may conceivably want to examine the ELF binary it produces. Fir instance, we may want to verify that it's got correct ELF sections in it, or examine dwarf data, etc. Maybe disassemble the code (we'd need another optional tool for that)

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

True, to an extent, but even potentially unstable tool output is better than nothing. I can think of using the output of `ptxas -v` to catch at least one kind of regressions we've ran into in the past.

I think we should figure out how such optional tools are supposed to work in lit and leave it up to users what they want to do with it. We don't know whether relying on external tool output is a good idea or not for all use cases.

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

Mostly syntax. It's yet another way to express "don't run if the feature is not available", as opposed to "substitute command with true and run".

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

Thank you. This is good to know.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121727