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

[Andrew Savonichev via Phabricator via llvm-commits]

asavonic added a comment.

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

> In D121727#3386172 <https://reviews.llvm.org/D121727#3386172>, @asavonic wrote:
>
>> The advantage here is that we can add these checks to all existing LIT tests and make sure that they indeed produce valid PTX code.
>
> Without ptxas available those tests would not do anything useful, would they? Why not just use `REQUIRES: ptxas` and avoid adding this special-case substitution of ptxas with `true`.
> I really don't see it buying us anything. There are no existing ptxas tests and adding `REQUIRES: ptxas` to the new ones sounds like the right thing to do.

The idea is to add ptxas RUN lines to existing ~200 LIT tests (see access-non-generic.ll as an example). Since we cannot add `REQUIRES: ptxas` to all of them, we can make these new checks optional.

>> For tests that require `ptxas` to function - `REQUIRES: ptxas` is definitely needed.
>
>
>
>> If we want to distinguish these two cases, then we can have two substitutions: `ptxas-verify` that expands to `ptxas -c -o /dev/null` (or to `true` if `ptxas` is not available), and just `ptxas` that requires the corresponding feature.
>
> I'm not convinced we need those two cases. I would prefer to keep things simple. If there's no ptxas available -> don't run ptxas tests.
> As a user I would be unpoleasantly surprised to see a test supposedly launching ptxas passing in my sandbox (because it actually happened ran 'true', without my knowledge), yet LLVM producing invalid PTX.
> I would much rather know upfront that PTX has not been tested with ptxas, rather than risking a false positive if ptxas was not enabled.

Yes, I agree. Unfortunately, I don't see any other way to reuse the existing tests and RUN lines.
If PXTAS_EXECUTABLE is set, nothing should be ignored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121727