[PATCH] D57243: GlobalISel: Allow bitcount ops to have different result type

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:756
+      return Legalized;
+    }
+
----------------
Hi Matt @arsenm,

Looks like adding `widenScalarDst` call for type index 0 is not enough to adjust the widening to the new type constraints in full. All the code below it that now would handle type index 1 widening appears to be written under the assumption that destination and source types are the same, and it widens both, source and the destination. It was perfectly fine an assumption before, but not anymore. If the source needs to be widened, it generates incorrect code.

Here's a unit test that reproduces one of the possible issues (thanks Aditya @aditya_nandakumar for coming up with it!):

```
diff --git a/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp b/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp
index 815f1e3d321..a5798693c08 100644
--- a/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp
+++ b/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp
@@ -341,6 +341,31 @@ TEST_F(GISelMITest, WidenBitCountingCTPOP) {
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
 
+// CTPOP widening.
+TEST_F(GISelMITest, WidenBitCountingCTPOP1) {
+  if (!TM)
+    return;
+
+  // Declare your legalization info
+  DefineLegalizerInfo(A, {
+    getActionDefinitionsBuilder(G_CTPOP).legalFor({{s16, s16}});
+  });
+  // Build
+  // Trunc it to s8.
+  LLT s8{LLT::scalar(8)};
+  LLT s16{LLT::scalar(16)};
+  auto MIBTrunc = B.buildTrunc(s8, Copies[0]);
+  auto MIBCTPOP = B.buildInstr(TargetOpcode::G_CTPOP, {s16}, {MIBTrunc});
+  AInfo Info(MF->getSubtarget());
+  DummyGISelObserver Observer;
+  LegalizerHelper Helper(*MF, Info, Observer, B);
+  EXPECT_TRUE(Helper.widenScalar(*MIBCTPOP, 1, s16) ==
+              LegalizerHelper::LegalizeResult::Legalized);
+
+
+  MF->dump();
+}
+
 // CTTZ_ZERO_UNDEF widening.
 TEST_F(GISelMITest, WidenBitCountingCTTZ_ZERO_UNDEF) {
   if (!TM)

```

Could you please take a look at this?


================
Comment at: test/CodeGen/AMDGPU/GlobalISel/legalize-ctlz.mir:78
+    ; CHECK: [[TRUNC:%[0-9]+]]:_(s16) = G_TRUNC [[COPY]](s32)
+    ; CHECK: [[CTLZ:%[0-9]+]]:_(s32) = G_CTLZ [[TRUNC]](s16)
+    ; CHECK: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 65535
----------------
Is this really legal?


================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp:338
   CHECK: [[Trunc:%[0-9]+]]:_(s8) = G_TRUNC [[Ctpop]]
   )";
 
----------------
Another example of the behavior - isn't widening for type index 1 supposed to widen only the source? But as could be seen here, it ends up widening both the source and the destination. The resulting code isn't broken largely because the source and destination types of the input instruction are the same.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57243/new/

https://reviews.llvm.org/D57243