[PATCH] D57243: GlobalISel: Allow bitcount ops to have different result type
Roman Tereshin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 4 16:14:03 PST 2019
rtereshin added inline comments.
================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:756
+ return Legalized;
+ }
+
----------------
arsenm wrote:
> arsenm wrote:
> > rtereshin wrote:
> > > 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
> > In general I don't think it's in general possible to avoid changing other type indexes. For example, FewerElements for a select's condition type requires doing the same FewerElements for the select type. The same applies for any vector cast. The two type indexes won't match, but they still need to have the same number of elements.
> >
> > I don't think these examples actually show this though. Note in these examples, these aren't producing the fully legalized operation. They're only doing one step, but need at least one more to legalize the operation
> A bigger problem I have is the order of legalization steps matters. The intermediate operations will be done in a different type if you specify the legalize actions for type index 1 before 0 vs. the other wya around. You wouldn't see the new operation with the narrower type if you legalized in the other order
> A bigger problem I have is the order of legalization steps matters. The intermediate operations will be done in a different type if you specify the legalize actions for type index 1 before 0 vs. the other wya around. You wouldn't see the new operation with the narrower type if you legalized in the other order
That's a very good point!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57243/new/
https://reviews.llvm.org/D57243
More information about the llvm-commits
mailing list