This is an archive of the discontinued LLVM Phabricator instance.

[SPIRV] Add simple tests to improve test coverage
ClosedPublic

Authored by andreytr on May 11 2022, 10:41 AM.

Diff Detail

Event Timeline

andreytr created this revision.May 11 2022, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 10:41 AM
andreytr requested review of this revision.May 11 2022, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 10:41 AM
MaskRay added inline comments.May 11 2022, 10:48 AM
llvm/test/CodeGen/SPIRV/TruncToBool.ll
4
7

;;

llvm/test/CodeGen/SPIRV/llvm-intrinsics/ctpop.ll
3

Keep just one blank line

5

[[#]] instead of {{[0-9]+}}

llvm/test/CodeGen/SPIRV/transcoding/OpSwitchEmpty.ll
2

You may use ;; for non-RUN non-CHECK lines.

Source: and Command: may be recognized as incorrect check prefixes by lit/FileCheck in the future. ;; makes it clear it is for pure comments.

zuban32 requested changes to this revision.May 11 2022, 2:28 PM

To me this subset of tests looks very counter-intuitive, I'd strongly prefer to have LITs related commits formed by some tests' common properties, e.g. '[SPIR-V] Add simple transcoding tests'. What was your motivation behind this particular subset, are these the only tests currently passing?

This revision now requires changes to proceed.May 11 2022, 2:28 PM

To me this subset of tests looks very counter-intuitive, I'd strongly prefer to have LITs related commits formed by some tests' common properties, e.g. '[SPIR-V] Add simple transcoding tests'. What was your motivation behind this particular subset, are these the only tests currently passing?

Yes, these are currently passing tests, translated from SPIRV-LLVM-Translator repository by Andrey.

andreytr added a comment.EditedMay 12 2022, 2:23 AM

Yes, these are the only tests currently passing.

@MaskRay :
Concerning "[[#]] instead of {{[0-9]+}}":
This style {{[0-9]+}} is widely used in SPIRV-LLVM-Translator's tests repository. So, it has to change nearly 150-200 converted tests in LLVM-SPIRV-Backend' tests repository. (?) Moreover, it will break link to original source SPIRV-LLVM-Translator's tests.
Is it required?

The rest things (whitespaces and comments) I will fix soon.

Concerning "[[#]] instead of {{[0-9]+}}":
This style {{[0-9]+}} is widely used in SPIRV-LLVM-Translator's tests repository. So, it has to change nearly 150-200 converted tests in LLVM-SPIRV-Backend' tests repository. (?) Moreover, it will break link to original source SPIRV-LLVM-Translator's tests.
Is it required?

Not strictly required, no. A quick grep in llvm/test shows 65 occurrences of [[#]] and 80388 of the regexp style.

On FileCheck's manual, this [1] is why [[#]] is equivalent and simpler, but also there are examples in the regexp form, which is also valid.

I'd say this could be a NFC change later to do all at once, but still isn't strictly necessary, it'd just be nice to start with a simpler pattern, then copy-and-paste later propagates it naturally.

[1] https://www.llvm.org/docs/CommandGuide/FileCheck.html#filecheck-numeric-substitution-blocks

andreytr updated this revision to Diff 428915.May 12 2022, 5:53 AM
andreytr retitled this revision from [SPIRV] Add SPIR-V tests to [SPIR-V] Add simple transcoding tests.

Fixed changes proposed by @MaskRay and @zuban32. I'm not sure about %[[PTR_ID:[0-9]+]] etc...
And what about build failures? It seems, these are not related to my patch...

andreytr marked 5 inline comments as done.May 12 2022, 5:56 AM

And what about build failures? It seems, these are not related to my patch...

Have you tested the main branch without your patches?

If the errors are identical, then it wasn't your patch.

If the errors have already been detected in buildbots, the ones responsible for fixing are probably already working on it / being notified. You could also just wait, rebase and try again later.

Fixed changes proposed by @MaskRay and @zuban32. I'm not sure about %[[PTR_ID:[0-9]+]] etc...
And what about build failures? It seems, these are not related to my patch...

You can use the capture pattern %[[#PTR_ID:]], then reference the capture with %[[#PTR_ID]].

llvm/test/CodeGen/SPIRV/optnone.ll
5
6

Fixed changes proposed by @MaskRay and @zuban32. I'm not sure about %[[PTR_ID:[0-9]+]] etc...
And what about build failures? It seems, these are not related to my patch...

You can use the capture pattern %[[#PTR_ID:]], then reference the capture with %[[#PTR_ID]].

Yes, I know. I mean, is it worth to do it? (There may be problems with updating tests from SPIRV-LLVM-Translator's tests repository in future.)

Fixed changes proposed by @MaskRay and @zuban32. I'm not sure about %[[PTR_ID:[0-9]+]] etc...
And what about build failures? It seems, these are not related to my patch...

You can use the capture pattern %[[#PTR_ID:]], then reference the capture with %[[#PTR_ID]].

Yes, I know. I mean, is it worth to do it? (There may be problems with updating tests from SPIRV-LLVM-Translator's tests repository in future.)

May be not for this case.

That said, I am more concerned with whether a party is willing to clean up tests during an upstreaming process. If not, then the review process in the upstream seems to lose value.

andreytr updated this revision to Diff 429018.May 12 2022, 11:00 AM

Fixed changes proposed by @MaskRay .

May be not for this case.

Late. I've already done it.

then the review process in the upstream seems to lose value.

Well...

andreytr marked 2 inline comments as done.May 12 2022, 11:04 AM
andreytr retitled this revision from [SPIR-V] Add simple transcoding tests to [SPIR-V] Add simple tests to improve test coverage.May 12 2022, 1:43 PM
andreytr updated this revision to Diff 429189.May 13 2022, 3:51 AM

Removed unneeded metadata.

MaskRay added a comment.EditedMay 13 2022, 10:41 AM

The style looks good to me, but hope someone familiar with SPIR-V can do a proper review.

llvm/test/CodeGen/SPIRV/TruncToBool.ll
24

Consider removing unneeded attributes to reduce noise of the test.

llvm/test/CodeGen/SPIRV/llvm-intrinsics/ctpop.ll
38

The excess spaces after i8 should be removed. You can see that Clang -emit-llvm -S doesn't add spaces (for at least the majority of targets).

41

#1 does not exist and should be removed.

llvm/test/CodeGen/SPIRV/transcoding/OpVectorExtractDynamic.ll
33

!3 == !4. Why keep both?

andreytr updated this revision to Diff 429314.May 13 2022, 11:49 AM

Fixed changes proposed by @MaskRay .

andreytr marked 4 inline comments as done.May 13 2022, 11:50 AM
MaskRay accepted this revision.May 13 2022, 12:12 PM

Thanks!

zuban32 added inline comments.May 16 2022, 10:35 AM
llvm/test/CodeGen/SPIRV/TruncToBool.ll
14

I think we should try get rid of unnecessary metadata as much as possible, I don't think kernel_arg_* matters for this test for example, since it only checks for emitting proper opcodes. The same is fair for some of the opencl.* MD or any other.

andreytr updated this revision to Diff 429804.May 16 2022, 11:57 AM

Removed metadata mentioned by @zuban32 .

andreytr marked an inline comment as done.May 16 2022, 11:58 AM
zuban32 added inline comments.May 16 2022, 1:39 PM
llvm/test/CodeGen/SPIRV/optnone.ll
17

Some (or all) of these attributes look redundant as well

llvm/test/CodeGen/SPIRV/transcoding/OpBitReverse_i32.ll
13

tbaa metadata is not needed too

arsenm added inline comments.May 16 2022, 1:55 PM
llvm/test/CodeGen/SPIRV/TruncToBool.ll
9–10

Generally tests use -mtriple on the command line and don't repeat the triple/datalayout in the IR

llvm/test/CodeGen/SPIRV/llvm-intrinsics/dynamic-memmove.ll
3

Negative tests are extremely fragile. Should check what is actually emitted here

llvm/test/CodeGen/SPIRV/llvm-intrinsics/experimental.noalias.scope.decl.ll
6

Ditto?

andreytr added inline comments.May 16 2022, 2:09 PM
llvm/test/CodeGen/SPIRV/TruncToBool.ll
9–10

Without "target triple = "spirv64-unknown-unknown" " it compiles x86 assembler.

arsenm added inline comments.May 16 2022, 2:11 PM
llvm/test/CodeGen/SPIRV/TruncToBool.ll
9–10

Yes, but you can move it to the -mtriple argument to llc instead of putting it in the source

andreytr updated this revision to Diff 429857.May 16 2022, 2:37 PM

Removed more metadata mentioned by @zuban32 and removed dynamic-memmove.ll and experimental.noalias.scope.decl.ll tests mentioned by @arsenm .

Removed more metadata mentioned by @zuban32 and removed dynamic-memmove.ll and experimental.noalias.scope.decl.ll tests mentioned by @arsenm .

I didn't mean to remove the test, I mean include the checks for the output

andreytr marked 2 inline comments as done.May 16 2022, 2:41 PM
andreytr added inline comments.
llvm/test/CodeGen/SPIRV/TruncToBool.ll
9–10

Will try it (later).

llvm/test/CodeGen/SPIRV/optnone.ll
17

Necessary (for passing) only "noinline" (it explicitly checked), but see comment "Check that optnone is correctly ignored...".

andreytr marked an inline comment as done.May 16 2022, 2:44 PM

Removed more metadata mentioned by @zuban32 and removed dynamic-memmove.ll and experimental.noalias.scope.decl.ll tests mentioned by @arsenm .

I didn't mean to remove the test, I mean include the checks for the output

We will think and discuss purpose of these tests... May be, include some variants in next patches.

zuban32 accepted this revision.May 17 2022, 8:28 AM

LGTM

This revision is now accepted and ready to land.May 17 2022, 8:28 AM
andreytr updated this revision to Diff 430099.May 17 2022, 9:46 AM

Moved triple string to llc's parameters.

andreytr marked 2 inline comments as done.May 17 2022, 9:47 AM

@arsenm , I've moved triple string to llc's arguments. Checked, that it produces the same assembler. Let's see this variant.

If OK, I'm ready to commit (merge) this patch. But I don't know, how to do this. It seems, I have no such privileges.

If OK, I'm ready to commit (merge) this patch. But I don't know, how to do this. It seems, I have no such privileges.

You need to write to Chris as described in https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

This revision was automatically updated to reflect the committed changes.
andreytr retitled this revision from [SPIR-V] Add simple tests to improve test coverage to [SPIRV] Add simple tests to improve test coverage.May 18 2022, 3:55 PM