Page MenuHomePhabricator

[M68k] (Patch 6/8) IR Tests
ClosedPublic

Authored by myhsu on Sep 27 2020, 10:22 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.

myhsu updated this revision to Diff 302186.Nov 1 2020, 4:30 PM
myhsu retitled this revision from [M68K] (Patch 6/8) IR Tests to [M68k] (Patch 6/8) IR Tests.

[NFC] Rename M680x0 to M68k

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

Will the extract-section util work on windows buildbots?

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.

myhsu added a comment.Nov 30 2020, 9:42 PM

Will the extract-section util work on windows buildbots?

We haven't tried that on Windows unfortunately, I believe our buildbot is only using Linux and maybe BSD now.

myhsu added a comment.Nov 30 2020, 9:53 PM

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.

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?

myhsu updated this revision to Diff 309424.Dec 3 2020, 5:54 PM
  • 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
myhsu updated this revision to Diff 309791.Dec 6 2020, 1:08 PM
  • Update test cases related to 8-bit multiplications.

Will the extract-section util work on windows buildbots?

We haven't tried that on Windows unfortunately, I believe our buildbot is only using Linux and maybe BSD 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.

myhsu updated this revision to Diff 310754.Dec 9 2020, 8:41 PM
  • 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
MaskRay added inline comments.Dec 14 2020, 7:19 PM
llvm/utils/extract-section.py
2

If not used, please drop it.

llvm-objcopy --dump-section=.foo=file a.o /dev/null

myhsu added inline comments.Dec 15 2020, 12:56 PM
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.

RKSimon added inline comments.Dec 17 2020, 3:46 AM
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.

rengolin accepted this revision.Dec 18 2020, 4:08 AM

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.

This revision is now accepted and ready to land.Dec 18 2020, 4:08 AM

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?

RKSimon added inline comments.Dec 21 2020, 1:41 AM
llvm/test/CodeGen/M68k/ASM/Arith/imul.ll
1 ↗(On Diff #310754)

why are you setting -O0 here?

myhsu updated this revision to Diff 320584.Feb 1 2021, 1:42 PM
myhsu marked 5 inline comments as done.
  • 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.

myhsu updated this revision to Diff 320682.Feb 1 2021, 10:54 PM
  • Reflecting changes from D88391 on optimizing multiplications
jrtc27 added a comment.Feb 7 2021, 1:05 PM

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.

RKSimon requested changes to this revision.Feb 8 2021, 1:17 AM

As requested by @jrtc27 please replace single use check-prefixes with the default 'CHECK' instead of x00/00 etc.

This revision now requires changes to proceed.Feb 8 2021, 1:17 AM
myhsu updated this revision to Diff 323441.Feb 12 2021, 12:18 PM
  • [NFC] Rename CHECK prefixes
    • Replace any standalone x00 prefix with normal CHECK
    • Capitalize custom prefix
myhsu updated this revision to Diff 323447.Feb 12 2021, 12:26 PM
  • [NFC] Remove reset of the x00 prefixes

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.

myhsu updated this revision to Diff 323671.Feb 14 2021, 10:56 PM
  • Use update_llc_test_checks.py to manage all the assembly tests

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.

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

RKSimon added inline comments.Feb 16 2021, 3:25 AM
llvm/test/CodeGen/M68k/ASM/Arith/imul-neg.ll
4 ↗(On Diff #323671)

Please can you raise a bug for these?

llvm/utils/extract-section.py
2

I have no objections to the extract-section.py as a temporary script but it would most likely have to go before m68k becomes non-experimental. @rengolin ?

rengolin added inline comments.Feb 16 2021, 3:48 AM
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.

myhsu marked 2 inline comments as done.Feb 17 2021, 11:58 PM
myhsu added inline comments.
llvm/test/CodeGen/M68k/ASM/Arith/imul-neg.ll
4 ↗(On Diff #323671)
llvm/utils/extract-section.py
2

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

Sounds like a good idea. Here is the bug: https://bugs.llvm.org/show_bug.cgi?id=49245

RKSimon accepted this revision.Feb 18 2021, 1:50 AM

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.

This revision is now accepted and ready to land.Feb 18 2021, 1:50 AM

I'd prefer to see:

  1. The ASM/ directory removed so Alloc/, Arith/ etc are directly under CodeGen/M68k, since those are the real CodeGen tests
  1. 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.
  1. 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

myhsu updated this revision to Diff 326535.Feb 25 2021, 3:45 PM
myhsu marked 10 inline comments as done.
  • Addressed feedbacks
llvm/test/CodeGen/M68k/ASM/CodeModel/medium-pie-global-access.ll
3 ↗(On Diff #323671)

thanks for the command!

myhsu updated this revision to Diff 326538.Feb 25 2021, 3:50 PM

[NFC] Combine README of test/CodeGen/M68k with the one in test/CodeGen/M68k/Encoding

jrtc27 added inline comments.Feb 25 2021, 4:00 PM
llvm/test/CodeGen/M68k/Encoding/Arith/Classes/MxBiArOp_FMI.mir
2

These look rather hand-formatted?

glaubitz added inline comments.Feb 28 2021, 6:53 PM
llvm/test/CodeGen/M68k/Encoding/Arith/Classes/MxBiArOp_FMI.mir
2

Hmm, what pointers do you have they are?

myhsu added inline comments.Feb 28 2021, 10:31 PM
llvm/test/CodeGen/M68k/Encoding/Arith/Classes/MxBiArOp_FMI.mir
2

These look rather hand-formatted?

Are you asking the "NOTE" line? It was generated by update_llc_test_checks.py

RKSimon added inline comments.Mar 1 2021, 12:31 AM
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?

myhsu updated this revision to Diff 327618.Mar 2 2021, 4:17 PM
myhsu marked 3 inline comments as done.

[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

jrtc27 accepted this revision.Mar 3 2021, 6:23 AM
This revision was landed with ongoing or failed builds.Mar 8 2021, 12:34 PM
This revision was automatically updated to reflect the committed changes.