All of the regression (IR) tests
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Part of the restructing of this patch series. Now all of the tests are centralized to this patch
Good to have these tests. I'd wait for the previous patches in the stack to be reviewed.
Nice set of tests, well separated, not a lot of pattern-match on the FileCheck side, which is good, asm and object emission.
No idea about the correctness wrt m68k, though. :)
I suggest you use update_llc_test_checks.py to automatically generate the CHECK lines for the IR tests (see the RISC-V backend which does this pretty diligently). Otherwise it's a real nuisance to update them after CodeGen changes that affect tests.
We haven't tried that on Windows unfortunately, I believe our buildbot is only using Linux and maybe BSD now.
Good point. However few tests are still failing, I guess we can't run this script to update the tests before we fix those right?
- Fix / Update failed test cases. Now we have 100% passing rate except one XFAIL, yeah!
- Most of the previous failed test cases were caused by less-optimal results from previous version of codegen. For example, the codegen for jump tables.
- The only XFAIL is caused by (selection DAG) ISel which failed to select one instruction that supposed to have 16-bit operand but a 8-bit one was given. It seems to take more time to fix it so just mark XFAIL for now
Is there anyway that you can replace the extract-section bash script with a extract-section.py python implementation? That should simplify windows support in the future.
- Rewrite utils/extract-section into utils/extract-section.py
- The new Python script is also more generic which doesn't have some M68k specific assumption (e.g. always group in 16-bits in hex mode) as the previous version
- Update M68k-local LIT config
llvm/utils/extract-section.py | ||
---|---|---|
2 | If not used, please drop it. llvm-objcopy --dump-section=.foo=file a.o /dev/null |
llvm/utils/extract-section.py | ||
---|---|---|
2 | If you're referring to this script as a whole, it is used extensively in M68k's test suite. This script is designed for printing certain section in textual format. So I'm afraid your llvm-objcopy command might not be sufficient. As the comment below described, this script is similar to llvm-readobj but the latter only support one output format (i.e. hex with lots of redundant info like ASCii decoding), where in many cases, we want other formats like hex string with different width and textual bits. |
llvm/test/CodeGen/M68k/ASM/Alloc/dyn_alloca_aligned.ll | ||
---|---|---|
1 ↗ | (On Diff #310754) | Very pedantic - but its more common just to use the default check prefix (CHECK) for a single run instead of a custom (x00) one. |
This LGTM now, thanks!
I agree with Simon that using the default CHECK in FileCheck is more common and easier to read than x00-NEXT.
However, I imagine this is like that because there would be other tests, for other sub-architectures (x10, x20, ...) that would be tested on the same file and therefore would need special CHECK lines on their own.
If that's true, than it should be fine to keep it like this for now, even if there's only one. Otherwise, you'd have to regex-replace all files now, and then later again, which is wasteful.
I see very little by way of testing the return side of the calling convention, and nothing for sret.
llvm/test/CodeGen/M68k/ASM/Alloc/dyn_alloca_aligned.ll | ||
---|---|---|
1 ↗ | (On Diff #310754) | Especially not one that's got lowercase letters in it... |
llvm/test/CodeGen/M68k/ASM/Arith/imul.ll | ||
93 ↗ | (On Diff #310754) | You don't mean that... |
llvm/test/CodeGen/M68k/ASM/CConv/c-call.ll | ||
15 ↗ | (On Diff #310754) | Do you need uwtable on any of these? |
llvm/test/CodeGen/M68k/ASM/Arith/imul.ll | ||
---|---|---|
1 ↗ | (On Diff #310754) | why are you setting -O0 here? |
- Addressed feedbacks
- Split the problems found in ASM/Arith/imul.ll into a separate test case ASM/Arith/imul-neg.ll and mark XFAIL for now
llvm/test/CodeGen/M68k/ASM/Arith/imul.ll | ||
---|---|---|
1 ↗ | (On Diff #310754) | I don't know the original intention. But after removing it, the last two functions failed their checks. Turns out there is a bug in SelectionDAGISel (which was not triggered in -O0 because the latter uses FastISel) that failed to lower certain cases into using neg over sub. Which, IIRC, the former is cheaper. I decided to split those two functions into a separated files and mark XFAIL. |
Most of these tests are still using bad check prefixes. Use CHECK if you only have one prefix in use, and if you need multiple then that's fine but make sure they match [A-Z0-9]+. Plus x00 doesn't make sense, it's m680x0 not m68x00.
As requested by @jrtc27 please replace single use check-prefixes with the default 'CHECK' instead of x00/00 etc.
- [NFC] Rename CHECK prefixes
- Replace any standalone x00 prefix with normal CHECK
- Capitalize custom prefix
As I've said before, please use update_llc_test_checks.py. Manually-maintained CHECK lines are a complete pain when making tree-wide changes.
llvm/utils/extract-section.py | ||
---|---|---|
2 | I'm with MaskRay, this script shouldn't exist, use the existing llvm tools. |
right, I didn't do this before because some tests were still failing, but now those problems are gone. Thanks for reminding.
I've also added m68k's asm regex for update_llc_test_checks.py.
llvm/utils/extract-section.py | ||
---|---|---|
2 | many of the tests here are checking against binary number, but IIRC the tools LLVM currently have are only able to print out hexadecimal number. The only solution (to get rid of this script) I can think of now is to rewrite all the checks in the tests, but that will be a non-trivial amount of works |
llvm/utils/extract-section.py | ||
---|---|---|
2 | At the very least removed before production, definitely. Though, I'm still undecided if we want to change all the tests or implement this in objdump & friends. To @jrtc27's point in another thread, using the existing update_test_checks script in LLVM would be a quick way to not have to rely on CHECK lines manually and may be a quick way out of this predicament. But I'm not sure how practical it would be to do this now to the entire set of tests. At least now we know that humans have looked at the tests and checked for consistency and correctness. They also express what is expected, rather than what is generated. It's entirely possible that the generations is not perfect (experimental back-end) and we'd be introducing comparisons to things in TODO/FIXME which doesn't help other developers when they break these tests. I'd prefer this to be a conscious step on its own, where people can look and make sure we're doing the right thing and expressing the right semantics, rather than a quick fix to merge a big patch series. My personal take is that it will be better to land something at least partially good sooner and have more people working directly in LLVM than holding this series for longer and keep the whole m68k community in a suspended state. Sure, the code is not perfect. There are stylistic and minor code issues Jessica so diligently spotted. Those can be quickly fixed and rebased, no worries. But bigger changes I'd prefer to keep it for after the first merge. Given that it's not yet clear what's the best alternative strategy, I'd create a bug for this, add all the context and opinions expressed here, and mark it as a dependency of the meta-bug for production m68k. |
llvm/test/CodeGen/M68k/ASM/Arith/imul-neg.ll | ||
---|---|---|
4 ↗ | (On Diff #323671) | |
llvm/utils/extract-section.py | ||
2 |
Sounds like a good idea. Here is the bug: https://bugs.llvm.org/show_bug.cgi?id=49245 |
LGTM
llvm/test/CodeGen/M68k/ASM/Arith/imul-neg.ll | ||
---|---|---|
4 ↗ | (On Diff #323671) | Its up to you, but I'd probably prefer to run the script here to show the current codegen, keep the FIXME and drop the XFAIL - its a poor codegen issue not an actual error. |
I'd prefer to see:
- The ASM/ directory removed so Alloc/, Arith/ etc are directly under CodeGen/M68k, since those are the real CodeGen tests
- the OBJ/ directory renamed to something like Encoding/ (the three-letter all-caps names are really ugly and not a style we tend to use), and the README updated to indicate that these are here solely because there is no integrated assembler support, and that the tests should be replaced by assembly->obj tests (which would live in llvm/test/MC/M68k) once that support exists.
- I don't get what PAS stands for, but that directory should go. Just move the test wherever it belongs, presumably with the normal CodeGen tests (i.e. 1 above).
That should be a fairly easy set of git mvs.
llvm/test/CodeGen/M68k/ASM/Arith/imul-neg.ll | ||
---|---|---|
4 ↗ | (On Diff #323671) | Agreed |
llvm/test/CodeGen/M68k/ASM/Arith/umul-with-overflow.ll | ||
2 ↗ | (On Diff #323671) | |
llvm/test/CodeGen/M68k/ASM/CodeModel/medium-pie-global-access.ll | ||
3 ↗ | (On Diff #323671) | These backslashes don't line up; either line them up everywhere or only leave one space before the backslash. I count 71 cases of multiple spaces before a backslash; some of those might be lined up already but they should be audited. I'd be inclined to just do a mass sed -i '/^[;#]/s/ *\\/ \\/ (and verify the result...) rather than going through and lining everything up (other backends don't bother). |
4 ↗ | (On Diff #323671) | You have 7 cases of this; these should be indented, typically by an additional 2 (most common, ~5200 cases) or 4 spaces (less common, ~1700 cases)). |
llvm/test/CodeGen/M68k/OBJ/Arith/Classes/MxBiArOp_FMI.mir | ||
3 ↗ | (On Diff #323671) | RUN: | is wrong, should have either 1 or 3 more spaces (i.e. it's 2 or 4 more spaces of indentation plus the space between : and the shell command). |
llvm/test/CodeGen/M68k/OBJ/Arith/Classes/MxBiArOp_RFRM.mir | ||
5 ↗ | (On Diff #323671) | You have loads of these badly-formatted cases |
- Addressed feedbacks
llvm/test/CodeGen/M68k/ASM/CodeModel/medium-pie-global-access.ll | ||
---|---|---|
3 ↗ | (On Diff #323671) | thanks for the command! |
llvm/test/CodeGen/M68k/Encoding/Arith/Classes/MxBiArOp_FMI.mir | ||
---|---|---|
2 | These look rather hand-formatted? |
llvm/test/CodeGen/M68k/Encoding/Arith/Classes/MxBiArOp_FMI.mir | ||
---|---|---|
2 | Hmm, what pointers do you have they are? |
llvm/test/CodeGen/M68k/Encoding/Arith/Classes/MxBiArOp_FMI.mir | ||
---|---|---|
2 |
Are you asking the "NOTE" line? It was generated by update_llc_test_checks.py |
llvm/test/CodeGen/M68k/Encoding/Arith/Classes/MxBiArOp_FMI.mir | ||
---|---|---|
2 | Yes but the actual test checks weren't generated by update_llc_test_checks.py so the NOTE probably shouldn't be there? |
[NFC] Remove all comments generated by update_llc_test_checks.py in MIR files
llvm/test/CodeGen/M68k/Encoding/Arith/Classes/MxBiArOp_FMI.mir | ||
---|---|---|
2 | That make sense, will remove |
These look rather hand-formatted?