Some of these accidentally disabled tests fail when enabled; left FIXMEs as I don't know what the correct results should be.
Found by the Rotten Green Tests project.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp | ||
---|---|---|
884–885 | It would be better to just update the checks for whatever is produced |
llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp | ||
---|---|---|
884–885 | I can try, but no promises the result will be meaningful and appropriate. This is totally not my area. |
I asked about this on llvm-dev but it seemed worth reporting here as well.
Some of the differences between the expected output (per the FileCheck directives) and the actual output appear to be little more than reordering of the instructions. However, oddly, in some cases the use of a value might precede its definition. For example, in NarrowSEXTINREG, the test is looking for this:
CHECK: [[T0:%[0-9]+]]:_(s16) = G_TRUNC CHECK: [[T1:%[0-9]+]]:_(s10) = G_TRUNC [[T0]]:_s(16) CHECK: [[T2:%[0-9]+]]:_(s10) = G_SEXT_INREG [[T1]]:_, 8 CHECK: [[T3:%[0-9]+]]:_(s16) = G_SEXT [[T2]]:_(s10)
But, what I see is this:
%5:_(s16) = G_SEXT %7:_(s10) %4:_(s16) = G_TRUNC %0:_(s64) %7:_(s10) = G_SEXT_INREG %6:_, 8 %6:_(s10) = G_TRUNC %4:_(s16)
So the SEXT uses the result of SEXT_INREG, which uses the result of the second TRUNC; these are all forward references. That doesn't seem correct; uses before defs? If there's a problem with instruction ordering, fixing that seems like its own issue, and would probably make many of the failures I'm seeing just go away. (Global ISel is very much not my area, so I'd appreciate it if someone else could figure out the ordering question.)
Or, if it's not a problem, I can just reorder the checks to match the output I'm seeing. But I wanted to verify that was the desirable result first.
Modified tests as recommended by Quentin on llvm-dev; I hope I got it right. Wasn't clear if the intent was to modify *all* tests in the file the same way (adding B.setInstr(foo)).
Updated remaining failures to match what is currently produced.
Thanks for fixing this, I just ran into the same problem after basing a new test off of one of these on an older branch and wondering why it passed before I'd written the result string. I did a bit of digging before finding this commit and I think part of the problem here is that gtest will call the virtual function SetUp() automatically but not setUp(). I think we should also rename setUp() to something that doesn't clash with gtest (createTargetAndModule, initLLVM, etc.)
Also gtest has a way to skip tests in recent versions: https://github.com/google/googletest/blob/master/googletest/test/gtest_skip_test.cc. It might be worth updating it so tests like this can report they were skipped instead of falsely claiming they passed.
It would be better to just update the checks for whatever is produced