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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm marked an inline comment as done.
arsenm added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:756
+      return Legalized;
+    }
+
----------------
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


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

https://reviews.llvm.org/D57243