Details
Diff Detail
Unit Tests
Event Timeline
| llvm/test/CodeGen/SPIRV/TruncToBool.ll | ||
|---|---|---|
| 3 | ||
| 6 | ;; | |
| llvm/test/CodeGen/SPIRV/llvm-intrinsics/ctpop.ll | ||
| 2 | Keep just one blank line | |
| 4 | [[#]] instead of {{[0-9]+}} | |
| llvm/test/CodeGen/SPIRV/transcoding/OpSwitchEmpty.ll | ||
| 1 | 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. | |
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.
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.
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
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.
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.
May be not for this case.
Late. I've already done it.
then the review process in the upstream seems to lose value.
Well...
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? | |
| 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. | |
| llvm/test/CodeGen/SPIRV/TruncToBool.ll | ||
|---|---|---|
| 10–11 | 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 | ||
| 4 | Negative tests are extremely fragile. Should check what is actually emitted here | |
| llvm/test/CodeGen/SPIRV/llvm-intrinsics/experimental.noalias.scope.decl.ll | ||
| 7 | Ditto? | |
| llvm/test/CodeGen/SPIRV/TruncToBool.ll | ||
|---|---|---|
| 10–11 | Without "target triple = "spirv64-unknown-unknown" " it compiles x86 assembler. | |
| llvm/test/CodeGen/SPIRV/TruncToBool.ll | ||
|---|---|---|
| 10–11 | Yes, but you can move it to the -mtriple argument to llc instead of putting it in the source | |
We will think and discuss purpose of these tests... May be, include some variants in next patches.
@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.