[RGT][GlobalISel] Add missing setUp() calls to legalizer unittests

[probinson]

Summary

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.

[arsenm]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

644–645

It would be better to just update the checks for whatever is produced

[probinson]

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

644–645

I can try, but no promises the result will be meaningful and appropriate. This is totally not my area.

[probinson]

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.

[arsenm]

In D95257#2530928, @probinson wrote:

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

Yes, this is plainly broken

[probinson]

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.

[qcolombet]

Looks good, thanks fixing the test!

[dsanders]

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.

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

TEST_F(AArch64GISelMITest, LowerBitCountingCTTZ2) {
   )";
 
   // Check
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 // CTPOP widening.
 TEST_F(AArch64GISelMITest, WidenBitCountingCTPOP1) {
+  setUp();
   if (!TM)
     return;
 
   // Declare your legalization info
   DefineLegalizerInfo(A, {
       getActionDefinitionsBuilder(G_CTPOP).legalFor({{s16, s16}});
     });
 
TEST_F(AArch64GISelMITest, WidenBitCountingCTPOP1) {
   CHECK: [[COPY:%[0-9]+]]:_(s16) = COPY [[CTPOP]]:_(s16)
   )";
 
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 // Test a strange case where the result is wider than the source
 TEST_F(AArch64GISelMITest, WidenBitCountingCTPOP2) {
+  setUp();
   if (!TM)
     return;
 
   // Declare your legalization info
   DefineLegalizerInfo(A, {
       getActionDefinitionsBuilder(G_CTPOP).legalFor({{s32, s16}});
     });
 
TEST_F(AArch64GISelMITest, WidenUSUBO) {
   CHECK: G_TRUNC [[SUB]]
   )";
 
   // Check
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 TEST_F(AArch64GISelMITest, FewerElementsAnd) {
+  // FIXME: Enabling this test fails.
+  // setUp();
   if (!TM)
     return;
 
   const LLT V2S32 = LLT::vector(2, 32);
   const LLT V5S32 = LLT::vector(5, 32);
 
   // Declare your legalization info
   DefineLegalizerInfo(A, {
TEST_F(AArch64GISelMITest, FewerElementsAnd) {
   CHECK: [[INSERT2:%[0-9]+]]:_(<5 x s32>) = G_INSERT [[INSERT1]]:_, [[AND2]]:_(s32), 128
   )";
 
   // Check
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 TEST_F(AArch64GISelMITest, MoreElementsAnd) {
+  // FIXME: Enabling this test fails.
+  // setUp();
   if (!TM)
     return;
 
   LLT s32 = LLT::scalar(32);
   LLT v2s32 = LLT::vector(2, 32);
   LLT v6s32 = LLT::vector(6, 32);
 
   LegalizerInfo LI;
TEST_F(AArch64GISelMITest, MoreElementsAnd) {
   CHECK: [[AND:%[0-9]+]]:_(<6 x s32>) = G_AND [[CONCAT0]]:_, [[CONCAT1]]:_
   CHECK: (<2 x s32>) = G_EXTRACT [[AND]]:_(<6 x s32>), 0
   )";
 
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 TEST_F(AArch64GISelMITest, FewerElementsPhi) {
+  // FIXME: Enabling this test fails.
+  // setUp();
   if (!TM)
     return;
 
   LLT s1 = LLT::scalar(1);
   LLT s32 = LLT::scalar(32);
   LLT s64 = LLT::scalar(64);
   LLT v2s32 = LLT::vector(2, 32);
   LLT v5s32 = LLT::vector(5, 32);
TEST_F(AArch64GISelMITest, FewerElementsPhi) {
   CHECK: [[USE_OP:%[0-9]+]]:_(<5 x s32>) = G_AND [[INSERT2]]:_, [[INSERT2]]:_
   )";
 
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 // FNEG expansion in terms of FSUB
 TEST_F(AArch64GISelMITest, LowerFNEG) {
+  // FIXME: Enabling this test fails.
+  // setUp();
   if (!TM)
     return;
 
   // Declare your legalization info
   DefineLegalizerInfo(A, {
     getActionDefinitionsBuilder(G_FSUB).legalFor({s64});
   });
 
TEST_F(AArch64GISelMITest, LowerFNEG) {
   CHECK: [[FSUB1:%[0-9]+]]:_(s64) = ninf G_FSUB [[CONST1]]:_, %0:_
   )";
 
   // Check
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 TEST_F(AArch64GISelMITest, LowerMinMax) {
+  // FIXME: Enabling this test fails.
+  // setUp();
   if (!TM)
     return;
 
   LLT s64 = LLT::scalar(64);
   LLT v2s32 = LLT::vector(2, 32);
 
   DefineLegalizerInfo(A, {
     getActionDefinitionsBuilder({G_SMIN, G_SMAX, G_UMIN, G_UMAX})
TEST_F(AArch64GISelMITest, LowerMinMax) {
   CHECK: [[VCMP3:%[0-9]+]]:_(<2 x s1>) = G_ICMP intpred(ugt), [[VEC0]]:_(<2 x s32>), [[VEC1]]:_
   CHECK: [[UMAXV:%[0-9]+]]:_(<2 x s32>) = G_SELECT [[VCMP3]]:_(<2 x s1>), [[VEC0]]:_, [[VEC1]]:_
   )";
 
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 TEST_F(AArch64GISelMITest, WidenScalarBuildVector) {
+  // FIXME: Enabling this test fails.
+  // setUp();
   if (!TM)
     return;
 
   LLT S32 = LLT::scalar(32);
   LLT S16 = LLT::scalar(16);
   LLT V2S16 = LLT::vector(2, S16);
   LLT V2S32 = LLT::vector(2, S32);
 
TEST_F(AArch64GISelMITest, WidenScalarBuildVector) {
 
   CHECK-NEXT: [[BV1:%[0-9]+]]:_(<2 x s16>) = G_BUILD_VECTOR_TRUNC [[EXT_K0_1]]:_(s32), [[EXT_K1_1]]:_(s32)
   )";
 
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 TEST_F(AArch64GISelMITest, LowerMergeValues) {
+  // FIXME: Enabling this test fails.
+  // setUp();
   if (!TM)
     return;
 
   const LLT S32 = LLT::scalar(32);
   const LLT S24 = LLT::scalar(24);
   const LLT S21 = LLT::scalar(21);
   const LLT S16 = LLT::scalar(16);
   const LLT S9 = LLT::scalar(9);
TEST_F(AArch64GISelMITest, LowerMergeValues) {
   [[OR:%[0-9]+]]:_(s32) = G_OR [[ZEXT_K16]]:_, [[SHL]]:_
   (s16) = G_TRUNC [[OR]]:_(s32)
   )";
 
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 TEST_F(AArch64GISelMITest, WidenScalarMergeValuesPointer) {
+  setUp();
   if (!TM)
     return;
 
   DefineLegalizerInfo(A, {});
 
   AInfo Info(MF->getSubtarget());
   DummyGISelObserver Observer;
   LegalizerHelper Helper(*MF, Info, Observer, B);
TEST_F(AArch64GISelMITest, WidenScalarMergeValuesPointer) {
    CHECK: [[OR:%[0-9]+]]:_(s64) = G_OR [[ZEXT_TRUNC0]]:_, [[SHL]]
    CHECK: [[INTTOPTR:%[0-9]+]]:_(p0) = G_INTTOPTR [[OR]]:_(s64)
   )";
 
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
 TEST_F(AArch64GISelMITest, WidenSEXTINREG) {
+  // FIXME: Enabling this test fails.
+  // setUp();
   if (!TM)
     return;
 
   // Declare your legalization info
   DefineLegalizerInfo(A, {
     getActionDefinitionsBuilder(G_SEXT_INREG).legalForTypeWithAnyImm({s64});
   });
   // Build Instr
TEST_F(AArch64GISelMITest, WidenSEXTINREG) {
   CHECK: [[T3:%[0-9]+]]:_(s32) = G_TRUNC [[T2]]:_(s64)
   )";
 
   // Check
   ASSERT_TRUE(CheckMachineFunction(*MF, CheckStr));
 }
 
 TEST_F(AArch64GISelMITest, NarrowSEXTINREG) {
+  // FIXME: Enabling this test fails.
+  // setUp();
   if (!TM)
     return;
 
   // Declare your legalization info, these aren't actually relevant to the test.
   DefineLegalizerInfo(A, {
     getActionDefinitionsBuilder(G_SEXT_INREG).legalForTypeWithAnyImm({s64});
   });
   // Build Instr
TEST_F(AArch64GISelMITest, NarrowSEXTINREG) {
   CHECK: [[T3:%[0-9]+]]:_(s16) = G_SEXT [[T2]]:_(s10)
   )";
 
   // Check
   ASSERT_TRUE(CheckMachineFunction(*MF, CheckStr));
 }
 
 TEST_F(AArch64GISelMITest, NarrowSEXTINREG2) {
+  setUp();
   if (!TM)
     return;
 
   // Declare your legalization info, these aren't actually relevant to the test.
   DefineLegalizerInfo(
       A, { getActionDefinitionsBuilder(G_SEXT_INREG).legalForTypeWithAnyImm({s64}); });
   // Build Instr
   auto MIB = B.buildInstr(
TEST_F(AArch64GISelMITest, NarrowSEXTINREG2) {
   CHECK: [[T7:%[0-9]+]]:_(s32) = G_MERGE_VALUES [[T1]]:_(s8), [[T5]]:_(s8), [[T6]]:_(s8), [[T6]]:_(s8)
   )";
 
   // Check
   ASSERT_TRUE(CheckMachineFunction(*MF, CheckStr));
 }
 
 TEST_F(AArch64GISelMITest, LowerSEXTINREG) {
+  setUp();
   if (!TM)
     return;
 
   // Declare your legalization info, these aren't actually relevant to the test.
   DefineLegalizerInfo(
       A, { getActionDefinitionsBuilder(G_SEXT_INREG).legalForTypeWithAnyImm({s64}); });
   // Build Instr
   auto MIB = B.buildInstr(