[GlobalISel] fix crash in narrowScalarExtract if DstRegs only has one register

[gargaroff]

Summary

When narrowing a scalar G_EXTRACT where the destination lines up perfectly with a single result of the emitted G_UNMERGE_VALUES a COPY should be emitted instead of unconditionally trying to emit a G_MERGE_VALUES.

[arsenm]

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

3688–3694

Could this just write the original result into DstReg and avoid the copy?

[gargaroff]

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

3688–3694

Hm, the only thing we could reasonably do here to avoid the copy is to change the operand of the defining instruction of DstRegs[0]. The code for this would look something like this:

else {
  MachineInstr *DefInst = MRI.getVRegDef(DstRegs[0]);
  assert(DefInst && "DstRegs[0] should have been defined");

  unsigned Idx = DefInst->getOpcode() == TargetOpcode::G_UNMERGE_VALUES ? OpStart / NarrowSize : 0;

  DefInst->getOperand(Idx).setReg(DstReg);
}

I mean it avoids the copy I guess, but not sure if that is really better here?

I also tried to rewrite the code to just use DstReg where we want to have it in the first place, but that just clutters the code with if-checks, so also not really nice.

Let me know what you think.

[arsenm]

I don't really love extra copies, especially since nothing is trying to get rid of them during legalization but it's not really important

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

if (ExtractOffset != 0 || SegSize != NarrowSize) {
       // A genuine extract is needed.
       SegReg = MRI.createGenericVirtualRegister(LLT::scalar(SegSize));
       MIRBuilder.buildExtract(SegReg, SrcRegs[i], ExtractOffset);
     }
 
     DstRegs.push_back(SegReg);
   }
 
   Register DstReg = MI.getOperand(0).getReg();
-  if(MRI.getType(DstReg).isVector())
+  if (MRI.getType(DstReg).isVector())
     MIRBuilder.buildBuildVector(DstReg, DstRegs);
-  else
+  else if (DstRegs.size() > 1)
     MIRBuilder.buildMerge(DstReg, DstRegs);
+  else
+    MIRBuilder.buildCopy(DstReg, DstRegs[0]);
   MI.eraseFromParent();
   return Legalized;
 }
 
 LegalizerHelper::LegalizeResult
 LegalizerHelper::narrowScalarInsert(MachineInstr &MI, unsigned TypeIdx,
                                     LLT NarrowTy) {
   // FIXME: Don't know how to handle secondary types yet.

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp

TEST_F(GISelMITest, LibcallFNearbyInt) {
   CHECK: BL &nearbyint
   CHECK: $q0 = COPY [[ANYEXT]]
   CHECK: BL &nearbyintl
   )";
 
   // Check
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
+
+TEST_F(GISelMITest, NarrowScalarExtract) {
+  setUp();
+  if (!TM)
+    return;
+
+  // Declare your legalization info
+  DefineLegalizerInfo(A, {
+    getActionDefinitionsBuilder(G_UNMERGE_VALUES).legalFor({{s32, s64}});
+    getActionDefinitionsBuilder(G_EXTRACT).legalFor({{s16, s32}});
+  });
+
+  LLT S16{LLT::scalar(16)};
+  LLT S32{LLT::scalar(32)};
+
+  auto MIBExtractS32 = B.buildExtract(S32, Copies[1], 32);
+  auto MIBExtractS16 = B.buildExtract(S16, Copies[1], 0);
+
+  AInfo Info(MF->getSubtarget());
+  DummyGISelObserver Observer;
+  LegalizerHelper Helper(*MF, Info, Observer, B);
+
+  EXPECT_EQ(LegalizerHelper::LegalizeResult::Legalized,
+            Helper.narrowScalar(*MIBExtractS32, 1, S32));
+
+  EXPECT_EQ(LegalizerHelper::LegalizeResult::Legalized,
+            Helper.narrowScalar(*MIBExtractS16, 1, S32));
+
+  const auto *CheckStr = R"(
+  CHECK: [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES
+  CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY [[UV1]]
+  CHECK: [[UV3:%[0-9]+]]:_(s32), [[UV4:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES
+  CHECK: [[EXTR:%[0-9]+]]:_(s16) = G_EXTRACT [[UV3]]:_(s32), 0
+  CHECK: [[COPY:%[0-9]+]]:_(s16) = COPY [[EXTR]]
+  )";
+
+  // Check
+  EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
+}
 } // namespace