GlobalISel: Fix CSE handling of buildConstant

This fixes two problems with CSE done in buildConstant. First, this
would hit an assert when used with a vector result type. Solve this by
allowing CSE on the vector elements, but not on the result vector for
now.

Second, this was also performing the CSE based on the input
ConstantInt pointer. The underlying buildConstant could potentially
convert the constant depending on the result type, giving in a
different ConstantInt*. Stop allowing the APInt and ConstantInt forms
from automatically casting to the result type to avoid any similar
problems in the future.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@353077 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index dcff4f546f2857b801ddcf7a1e040933e6865117..9e47880e2804e4c7ee0ec9137b212aabdfb62c6e 100644 (file)
--- a/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -585,6 +585,7 @@ public:
   ///
   /// \return The newly created instruction.
   MachineInstrBuilder buildConstant(const DstOp &Res, int64_t Val);
+  MachineInstrBuilder buildConstant(const DstOp &Res, const APInt &Val);
 
   /// Build and insert \p Res = G_FCONSTANT \p Val
   ///
@@ -710,6 +711,11 @@ public:
   MachineInstrBuilder buildBuildVector(const DstOp &Res,
                                        ArrayRef<unsigned> Ops);
 
+  /// Build and insert \p Res = G_BUILD_VECTOR with \p Src0 replicated to fill
+  /// the number of elements
+  MachineInstrBuilder buildSplatVector(const DstOp &Res,
+                                       const SrcOp &Src);
+
   /// Build and insert \p Res = G_BUILD_VECTOR_TRUNC \p Op0, ...
   ///
   /// G_BUILD_VECTOR_TRUNC creates a vector value from multiple scalar registers

diff --git a/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp b/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
index 53675e8f83e78da6004a590a4d34e814da4a8673..ed2c95c22cec391670f8e7eb57a27b2ec8b29f16 100644 (file)
--- a/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
+++ b/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
@@ -194,6 +194,12 @@ MachineInstrBuilder CSEMIRBuilder::buildConstant(const DstOp &Res,
   constexpr unsigned Opc = TargetOpcode::G_CONSTANT;
   if (!canPerformCSEForOpc(Opc))
     return MachineIRBuilder::buildConstant(Res, Val);
+
+  // For vectors, CSE the element only for now.
+  LLT Ty = Res.getLLTTy(*getMRI());
+  if (Ty.isVector())
+    return buildSplatVector(Res, buildConstant(Ty.getElementType(), Val));
+
   FoldingSetNodeID ID;
   GISelInstProfileBuilder ProfBuilder(ID, *getMRI());
   void *InsertPos = nullptr;
@@ -205,6 +211,7 @@ MachineInstrBuilder CSEMIRBuilder::buildConstant(const DstOp &Res,
     // Handle generating copies here.
     return generateCopiesIfRequired({Res}, MIB);
   }
+
   MachineInstrBuilder NewMIB = MachineIRBuilder::buildConstant(Res, Val);
   return memoizeMI(NewMIB, InsertPos);
 }
@@ -214,6 +221,12 @@ MachineInstrBuilder CSEMIRBuilder::buildFConstant(const DstOp &Res,
   constexpr unsigned Opc = TargetOpcode::G_FCONSTANT;
   if (!canPerformCSEForOpc(Opc))
     return MachineIRBuilder::buildFConstant(Res, Val);
+
+  // For vectors, CSE the element only for now.
+  LLT Ty = Res.getLLTTy(*getMRI());
+  if (Ty.isVector())
+    return buildSplatVector(Res, buildFConstant(Ty.getElementType(), Val));
+
   FoldingSetNodeID ID;
   GISelInstProfileBuilder ProfBuilder(ID, *getMRI());
   void *InsertPos = nullptr;

diff --git a/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp b/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
index 656352d1208af8dda1cbd99da86ac5a94244f526..8fa818afd70b3576bdc7dcded91b28169ba9f8ca 100644 (file)
--- a/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
+++ b/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
@@ -244,38 +244,26 @@ MachineInstrBuilder MachineIRBuilder::buildConstant(const DstOp &Res,
                                                     const ConstantInt &Val) {
   LLT Ty = Res.getLLTTy(*getMRI());
   LLT EltTy = Ty.getScalarType();
-
-  const ConstantInt *NewVal = &Val;
-  if (EltTy.getSizeInBits() != Val.getBitWidth()) {
-    NewVal = ConstantInt::get(
-      getMF().getFunction().getContext(),
-      Val.getValue().sextOrTrunc(EltTy.getSizeInBits()));
-  }
+  assert(EltTy.getScalarSizeInBits() == Val.getBitWidth() &&
+         "creating constant with the wrong size");
 
   if (Ty.isVector()) {
-    unsigned EltReg = getMRI()->createGenericVirtualRegister(EltTy);
-    buildInstr(TargetOpcode::G_CONSTANT)
-      .addDef(EltReg)
-      .addCImm(NewVal);
-
-    auto MIB = buildInstr(TargetOpcode::G_BUILD_VECTOR);
-    Res.addDefToMIB(*getMRI(), MIB);
-
-    for (unsigned I = 0, E = Ty.getNumElements(); I != E; ++I)
-      MIB.addUse(EltReg);
-    return MIB;
+    auto Const = buildInstr(TargetOpcode::G_CONSTANT)
+    .addDef(getMRI()->createGenericVirtualRegister(EltTy))
+    .addCImm(&Val);
+    return buildSplatVector(Res, Const);
   }
 
-  auto MIB = buildInstr(TargetOpcode::G_CONSTANT);
-  Res.addDefToMIB(*getMRI(), MIB);
-  MIB.addCImm(NewVal);
-  return MIB;
+  auto Const = buildInstr(TargetOpcode::G_CONSTANT);
+  Res.addDefToMIB(*getMRI(), Const);
+  Const.addCImm(&Val);
+  return Const;
 }
 
 MachineInstrBuilder MachineIRBuilder::buildConstant(const DstOp &Res,
                                                     int64_t Val) {
   auto IntN = IntegerType::get(getMF().getFunction().getContext(),
-                               Res.getLLTTy(*getMRI()).getSizeInBits());
+                               Res.getLLTTy(*getMRI()).getScalarSizeInBits());
   ConstantInt *CI = ConstantInt::get(IntN, Val, true);
   return buildConstant(Res, *CI);
 }
@@ -283,28 +271,32 @@ MachineInstrBuilder MachineIRBuilder::buildConstant(const DstOp &Res,
 MachineInstrBuilder MachineIRBuilder::buildFConstant(const DstOp &Res,
                                                      const ConstantFP &Val) {
   LLT Ty = Res.getLLTTy(*getMRI());
+  LLT EltTy = Ty.getScalarType();
+
+  assert(APFloat::getSizeInBits(Val.getValueAPF().getSemantics())
+         == EltTy.getSizeInBits() &&
+         "creating fconstant with the wrong size");
 
   assert(!Ty.isPointer() && "invalid operand type");
 
   if (Ty.isVector()) {
-    unsigned EltReg
-      = getMRI()->createGenericVirtualRegister(Ty.getElementType());
-    buildInstr(TargetOpcode::G_FCONSTANT)
-      .addDef(EltReg)
-      .addFPImm(&Val);
-
-    auto MIB = buildInstr(TargetOpcode::G_BUILD_VECTOR);
-    Res.addDefToMIB(*getMRI(), MIB);
-
-    for (unsigned I = 0, E = Ty.getNumElements(); I != E; ++I)
-      MIB.addUse(EltReg);
-    return MIB;
+    auto Const = buildInstr(TargetOpcode::G_FCONSTANT)
+    .addDef(getMRI()->createGenericVirtualRegister(EltTy))
+    .addFPImm(&Val);
+
+    return buildSplatVector(Res, Const);
   }
 
-  auto MIB = buildInstr(TargetOpcode::G_FCONSTANT);
-  Res.addDefToMIB(*getMRI(), MIB);
-  MIB.addFPImm(&Val);
-  return MIB;
+  auto Const = buildInstr(TargetOpcode::G_FCONSTANT);
+  Res.addDefToMIB(*getMRI(), Const);
+  Const.addFPImm(&Val);
+  return Const;
+}
+
+MachineInstrBuilder MachineIRBuilder::buildConstant(const DstOp &Res,
+                                                    const APInt &Val) {
+  ConstantInt *CI = ConstantInt::get(getMF().getFunction().getContext(), Val);
+  return buildConstant(Res, *CI);
 }
 
 MachineInstrBuilder MachineIRBuilder::buildFConstant(const DstOp &Res,
@@ -312,7 +304,7 @@ MachineInstrBuilder MachineIRBuilder::buildFConstant(const DstOp &Res,
   LLT DstTy = Res.getLLTTy(*getMRI());
   auto &Ctx = getMF().getFunction().getContext();
   auto *CFP =
-      ConstantFP::get(Ctx, getAPFloatFromSize(Val, DstTy.getSizeInBits()));
+      ConstantFP::get(Ctx, getAPFloatFromSize(Val, DstTy.getScalarSizeInBits()));
   return buildFConstant(Res, *CFP);
 }
 
@@ -557,6 +549,12 @@ MachineInstrBuilder MachineIRBuilder::buildBuildVector(const DstOp &Res,
   return buildInstr(TargetOpcode::G_BUILD_VECTOR, Res, TmpVec);
 }
 
+MachineInstrBuilder MachineIRBuilder::buildSplatVector(const DstOp &Res,
+                                                       const SrcOp &Src) {
+  SmallVector<SrcOp, 8> TmpVec(Res.getLLTTy(*getMRI()).getNumElements(), Src);
+  return buildInstr(TargetOpcode::G_BUILD_VECTOR, Res, TmpVec);
+}
+
 MachineInstrBuilder
 MachineIRBuilder::buildBuildVectorTrunc(const DstOp &Res,
                                         ArrayRef<unsigned> Ops) {

diff --git a/unittests/CodeGen/GlobalISel/CSETest.cpp b/unittests/CodeGen/GlobalISel/CSETest.cpp
index cd6f99bf6e414f20b9cadba5aa29bf053b4ded60..02627cae164fe0eb258ee2f127ebf06916b8abf5 100644 (file)
--- a/unittests/CodeGen/GlobalISel/CSETest.cpp
+++ b/unittests/CodeGen/GlobalISel/CSETest.cpp
@@ -25,6 +25,7 @@ TEST_F(GISelMITest, TestCSE) {
   CSEInfo.analyze(*MF);
   B.setCSEInfo(&CSEInfo);
   CSEMIRBuilder CSEB(B.getState());
+
   CSEB.setInsertPt(*EntryMBB, EntryMBB->begin());
   unsigned AddReg = MRI->createGenericVirtualRegister(s16);
   auto MIBAddCopy =
@@ -54,6 +55,18 @@ TEST_F(GISelMITest, TestCSE) {
   EXPECT_TRUE(&*MIBFP0 == &*MIBFP0_1);
   CSEInfo.print();
 
+  // Make sure buildConstant with a vector type doesn't crash, and the elements
+  // CSE.
+  auto Splat0 = CSEB.buildConstant(LLT::vector(2, s32), 0);
+  EXPECT_EQ(TargetOpcode::G_BUILD_VECTOR, Splat0->getOpcode());
+  EXPECT_EQ(Splat0->getOperand(1).getReg(), Splat0->getOperand(2).getReg());
+  EXPECT_EQ(&*MIBCst, MRI->getVRegDef(Splat0->getOperand(1).getReg()));
+
+  auto FSplat = CSEB.buildFConstant(LLT::vector(2, s32), 1.0);
+  EXPECT_EQ(TargetOpcode::G_BUILD_VECTOR, FSplat->getOpcode());
+  EXPECT_EQ(FSplat->getOperand(1).getReg(), FSplat->getOperand(2).getReg());
+  EXPECT_EQ(&*MIBFP0, MRI->getVRegDef(FSplat->getOperand(1).getReg()));
+
   // Check G_UNMERGE_VALUES
   auto MIBUnmerge = CSEB.buildUnmerge({s32, s32}, Copies[0]);
   auto MIBUnmerge2 = CSEB.buildUnmerge({s32, s32}, Copies[0]);

diff --git a/unittests/CodeGen/GlobalISel/MachineIRBuilderTest.cpp b/unittests/CodeGen/GlobalISel/MachineIRBuilderTest.cpp
index ca25ec54d6d8a71a40fe80bb6aba8db68db90037..9a2533159bedaf542ba075c62013215360433eb1 100644 (file)
--- a/unittests/CodeGen/GlobalISel/MachineIRBuilderTest.cpp
+++ b/unittests/CodeGen/GlobalISel/MachineIRBuilderTest.cpp
@@ -13,9 +13,6 @@ TEST_F(GISelMITest, TestBuildConstantFConstant) {
   if (!TM)
     return;
 
-  MachineIRBuilder B(*MF);
-  B.setInsertPt(*EntryMBB, EntryMBB->begin());
-
   B.buildConstant(LLT::scalar(32), 42);
   B.buildFConstant(LLT::scalar(32), 1.0);
 
@@ -27,9 +24,44 @@ TEST_F(GISelMITest, TestBuildConstantFConstant) {
   CHECK: [[FCONST0:%[0-9]+]]:_(s32) = G_FCONSTANT float 1.000000e+00
   CHECK: [[CONST1:%[0-9]+]]:_(s32) = G_CONSTANT i32 99
   CHECK: [[VEC0:%[0-9]+]]:_(<2 x s32>) = G_BUILD_VECTOR [[CONST1]]:_(s32), [[CONST1]]:_(s32)
-  CHECK: [[FCONST1:%[0-9]+]]:_(s32) = G_FCONSTANT double 2.000000e+00
+  CHECK: [[FCONST1:%[0-9]+]]:_(s32) = G_FCONSTANT float 2.000000e+00
   CHECK: [[VEC1:%[0-9]+]]:_(<2 x s32>) = G_BUILD_VECTOR [[FCONST1]]:_(s32), [[FCONST1]]:_(s32)
   )";
 
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
+
+
+#ifdef GTEST_HAS_DEATH_TEST
+#ifndef NDEBUG
+
+TEST_F(GISelMITest, TestBuildConstantFConstantDeath) {
+  if (!TM)
+    return;
+
+  LLVMContext &Ctx = MF->getFunction().getContext();
+  APInt APV32(32, 12345);
+
+  // Test APInt version breaks
+  EXPECT_DEATH(B.buildConstant(LLT::scalar(16), APV32),
+               "creating constant with the wrong size");
+  EXPECT_DEATH(B.buildConstant(LLT::vector(2, 16), APV32),
+               "creating constant with the wrong size");
+
+  // Test ConstantInt version breaks
+  ConstantInt *CI = ConstantInt::get(Ctx, APV32);
+  EXPECT_DEATH(B.buildConstant(LLT::scalar(16), *CI),
+               "creating constant with the wrong size");
+  EXPECT_DEATH(B.buildConstant(LLT::vector(2, 16), *CI),
+               "creating constant with the wrong size");
+
+  APFloat DoubleVal(APFloat::IEEEdouble());
+  ConstantFP *CF = ConstantFP::get(Ctx, DoubleVal);
+  EXPECT_DEATH(B.buildFConstant(LLT::scalar(16), *CF),
+               "creating fconstant with the wrong size");
+  EXPECT_DEATH(B.buildFConstant(LLT::vector(2, 16), *CF),
+               "creating fconstant with the wrong size");
+}
+
+#endif
+#endif