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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added a subscriber: qcolombet.
rtereshin added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:756
+      return Legalized;
+    }
+
----------------
arsenm wrote:
> rtereshin wrote:
> > 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?
> Try r353102
Hi Matt,

That's much better, thank you!

I feel a little bit divided though about the fact that in some cases widening one type index ends up changing the type of the other type index.

For instance, in this case widening type index 1 ends up also widening type index 0:
```lang=c++
// CTLZ widening.
TEST_F(GISelMITest, WidenBitCountingCTLZ) {
  if (!TM)
    return;

  // Declare your legalization info
  DefineLegalizerInfo(A, {
    getActionDefinitionsBuilder(G_CTLZ).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 MIBCTLZ = B.buildInstr(TargetOpcode::G_CTLZ, {s8}, {MIBTrunc}); // We start with the type signature {s8, s8}
  AInfo Info(MF->getSubtarget());
  DummyGISelObserver Observer;
  LegalizerHelper Helper(*MF, Info, Observer, B);
  EXPECT_TRUE(Helper.widenScalar(*MIBCTLZ, 1, s16) ==  // We ask the API to widen type index 1 to s16, probably expecting type signature {s8, s16} as a result
              LegalizerHelper::LegalizeResult::Legalized);

  auto CheckStr = R"(
  CHECK: [[Trunc:%[0-9]+]]:_(s8) = G_TRUNC
  CHECK: [[Zext:%[0-9]+]]:_(s16) = G_ZEXT [[Trunc]]
  CHECK: [[Ctlz:%[0-9]+]]:_(s16) = G_CTLZ [[Zext]]  # The API ends up widening not just type index 1, but also type index 0 with the resulting {s16, s16} type signature
  CHECK: [[Cst8:%[0-9]+]]:_(s16) = G_CONSTANT i16 8
  CHECK: [[Sub:%[0-9]+]]:_(s16) = G_SUB [[Ctlz]]:_, [[Cst8]]:_
  CHECK: [[Trunc:%[0-9]+]]:_(s8) = G_TRUNC [[Sub]]
  )";

  // Check
  EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
}
```

In this other case widening type index 1 ends up also narrowing type index 0:
```lang=c++
// Test a strange case where the result is wider than the source
TEST_F(GISelMITest, WidenBitCountingCTPOP2) {
  if (!TM)
    return;

  // Declare your legalization info
  DefineLegalizerInfo(A, {
      getActionDefinitionsBuilder(G_CTPOP).legalFor({{s32, s16}});
    });

  // Build
  // Trunc it to s8.
  LLT s8{LLT::scalar(8)};
  LLT s16{LLT::scalar(16)};
  LLT s32{LLT::scalar(32)};
  auto MIBTrunc = B.buildTrunc(s8, Copies[0]);
  auto MIBCTPOP = B.buildInstr(TargetOpcode::G_CTPOP, {s32}, {MIBTrunc}); // We start with type signature {s32, s8}
  AInfo Info(MF->getSubtarget());
  DummyGISelObserver Observer;
  LegalizerHelper Helper(*MF, Info, Observer, B);
  EXPECT_EQ(LegalizerHelper::LegalizeResult::Legalized,  // We ask the API to widen type index 1 to s16, probably expecting {s32, s16} as a result
            Helper.widenScalar(*MIBCTPOP, 1, s16));

  auto CheckStr = R"(
  CHECK: [[TRUNC:%[0-9]+]]:_(s8) = G_TRUNC %0:_(s64)
  CHECK: [[ZEXT:%[0-9]+]]:_(s16) = G_ZEXT [[TRUNC]]:_(s8)
  CHECK: [[CTPOP:%[0-9]+]]:_(s16) = G_CTPOP [[ZEXT]]  # API ends up narrowing the type index 0 in the process with {s16, s16} type signature as a result.
  CHECK: [[COPY:%[0-9]+]]:_(s32) = G_ZEXT [[CTPOP]]:_(s16)
  )";

  EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
}
```

I talked to @aditya_nandakumar, @volkan, and @qcolombet offline and it appears that we don't have any contract about what pre-defined operations like widenScalar, narrowScalar etc. are allowed to do with the type indices not included in the request, but I think it maybe worth introducing such a contract. Specifically the one that would state that it's guaranteed that these generic, pre-defined operations aren't allowed to change the types of the type indices except the one(s) explicitly included in the request.

I think not having these operations following such a contract makes their behavior quite unexpected for anyone who focuses on writing legalizations rules for their target w/o having an extensive knowledge about how exactly the implementation of the pre-defined API-provided operations looks like. In particular, I think it might make it easier for the users to accidentally introduce loops in legalization rules, and harder for them to understand where they are coming from.

Please let me know what do you think!

Thanks,
Roman


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

https://reviews.llvm.org/D57243