GlobalISel: Fix CSEMIRBuilder mishandling constant folds of vectors

[arsenm]

Summary

This was ignoring the requested result register, resulting in a
missing def when this happened in the IRTranslator. Fixes some crashes
and verifier errors at -O0.

Alternatively we could pass DstOps to the constant fold functions.

[foad]

Alternatively we could pass DstOps to the constant fold functions.

If that would avoid building a copy when DstOp is not a register then it sounds better to me.

[paquette]

+1 for avoiding the copy if possible, otherwise looks fine to me?

[aemerson]

I have no strong preference of whether we emit a copy or not FWIW.

[arsenm]

da7282276385f82092fed4e73373bbfaf7cd8331

[arsenm]

In D117026#3234956, @paquette wrote:

+1 for avoiding the copy if possible, otherwise looks fine to me?

DstOps is part of the MachineIRBuilder API, so I'm not sure it should be spread to Utils. However since this is using it anyway, why is ConstantFoldVectorBinop in Utils? We could either move the whole function into the MIRBuilder, or have the utils function return a vector of APInt

[foad]

or have the utils function return a vector of APInt

I have tried that in D117758.

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

 Optional<APFloat> ConstantFoldFPBinOp(unsigned Opcode, const Register Op1,
                                       const Register Op2,
                                       const MachineRegisterInfo &MRI);
 
 /// Tries to constant fold a vector binop with sources \p Op1 and \p Op2.
 /// If successful, returns the G_BUILD_VECTOR representing the folded vector
 /// constant. \p MIB should have an insertion point already set to create new
 /// G_CONSTANT instructions as needed.
-Optional<MachineInstr *>
-ConstantFoldVectorBinop(unsigned Opcode, const Register Op1, const Register Op2,
-                        const MachineRegisterInfo &MRI, MachineIRBuilder &MIB);
+Register ConstantFoldVectorBinop(unsigned Opcode, const Register Op1,
+                                 const Register Op2,
+                                 const MachineRegisterInfo &MRI,
+                                 MachineIRBuilder &MIB);
 
 Optional<APInt> ConstantFoldExtOp(unsigned Opcode, const Register Op1,
                                   uint64_t Imm, const MachineRegisterInfo &MRI);
 
 Optional<APFloat> ConstantFoldIntToFloat(unsigned Opcode, LLT DstTy,
                                          Register Src,
                                          const MachineRegisterInfo &MRI);
 

llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp

MachineInstrBuilder CSEMIRBuilder::buildInstr(unsigned Opc,
   case TargetOpcode::G_SDIV:
   case TargetOpcode::G_UREM:
   case TargetOpcode::G_SREM: {
     // Try to constant fold these.
     assert(SrcOps.size() == 2 && "Invalid sources");
     assert(DstOps.size() == 1 && "Invalid dsts");
     if (SrcOps[0].getLLTTy(*getMRI()).isVector()) {
       // Try to constant fold vector constants.
-      auto VecCst = ConstantFoldVectorBinop(
+      Register VecCst = ConstantFoldVectorBinop(
           Opc, SrcOps[0].getReg(), SrcOps[1].getReg(), *getMRI(), *this);
       if (VecCst)
-        return MachineInstrBuilder(getMF(), *VecCst);
+        return buildCopy(DstOps[0], VecCst);
       break;
     }
     if (Optional<APInt> Cst = ConstantFoldBinOp(Opc, SrcOps[0].getReg(),
                                                 SrcOps[1].getReg(), *getMRI()))
       return buildConstant(DstOps[0], *Cst);
     break;
   }
   case TargetOpcode::G_SEXT_INREG: {

llvm/lib/CodeGen/GlobalISel/Utils.cpp

case TargetOpcode::G_FMAXNUM_IEEE:
     break;
   default:
     break;
   }
 
   return None;
 }
 
-Optional<MachineInstr *>
-llvm::ConstantFoldVectorBinop(unsigned Opcode, const Register Op1,
+Register llvm::ConstantFoldVectorBinop(unsigned Opcode, const Register Op1,
                                        const Register Op2,
                                        const MachineRegisterInfo &MRI,
                                        MachineIRBuilder &MIB) {
   auto *SrcVec1 = getOpcodeDef<GBuildVector>(Op1, MRI);
   if (!SrcVec1)
-    return None;
+    return Register();
   auto *SrcVec2 = getOpcodeDef<GBuildVector>(Op2, MRI);
   if (!SrcVec2)
-    return None;
+    return Register();
 
   const LLT EltTy = MRI.getType(SrcVec1->getSourceReg(0));
 
   SmallVector<Register, 16> FoldedElements;
   for (unsigned Idx = 0, E = SrcVec1->getNumSources(); Idx < E; ++Idx) {
     auto MaybeCst = ConstantFoldBinOp(Opcode, SrcVec1->getSourceReg(Idx),
                                       SrcVec2->getSourceReg(Idx), MRI);
     if (!MaybeCst)
-      return None;
+      return Register();
     auto FoldedCstReg = MIB.buildConstant(EltTy, *MaybeCst).getReg(0);
     FoldedElements.emplace_back(FoldedCstReg);
   }
   // Create the new vector constant.
   auto CstVec =
       MIB.buildBuildVector(MRI.getType(SrcVec1->getReg(0)), FoldedElements);
-  return &*CstVec;
+  return CstVec.getReg(0);
 }
 
 bool llvm::isKnownNeverNaN(Register Val, const MachineRegisterInfo &MRI,
                            bool SNaN) {
   const MachineInstr *DefMI = MRI.getVRegDef(Val);
   if (!DefMI)
     return false;
 

llvm/test/CodeGen/AMDGPU/GlobalISel/combiner-crash.ll

+; RUN: llc -O0 -global-isel -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -o - %s
+
+define amdgpu_kernel void @test_long_add4() {
+entry:
+  %add = add <4 x i64> zeroinitializer, zeroinitializer
+  store <4 x i64> %add, <4 x i64> addrspace(1)* null, align 32
+  ret void
+}

llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-constant-fold-vector-op.ll

+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -O0 -global-isel -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -stop-after=irtranslator -o - %s | FileCheck %s
+
+; The CSEMIRBuilder was mishandling the result operand when constant
+; folding a vector operation, resulting in a missing def for the
+; stored value.
+define amdgpu_kernel void @constant_fold_vector_add() {
+  ; CHECK-LABEL: name: constant_fold_vector_add
+  ; CHECK: bb.1.entry:
+  ; CHECK-NEXT:   [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
+  ; CHECK-NEXT:   [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s64>) = G_BUILD_VECTOR [[C]](s64), [[C]](s64), [[C]](s64), [[C]](s64)
+  ; CHECK-NEXT:   [[C1:%[0-9]+]]:_(p1) = G_CONSTANT i64 0
+  ; CHECK-NEXT:   [[C2:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
+  ; CHECK-NEXT:   [[BUILD_VECTOR1:%[0-9]+]]:_(<4 x s64>) = G_BUILD_VECTOR [[C2]](s64), [[C2]](s64), [[C2]](s64), [[C2]](s64)
+  ; CHECK-NEXT:   [[COPY:%[0-9]+]]:_(<4 x s64>) = COPY [[BUILD_VECTOR1]](<4 x s64>)
+  ; CHECK-NEXT:   G_STORE [[COPY]](<4 x s64>), [[C1]](p1) :: (store (<4 x s64>) into `<4 x i64> addrspace(1)* null`, addrspace 1)
+  ; CHECK-NEXT:   S_ENDPGM 0
+entry:
+  %add = add <4 x i64> zeroinitializer, zeroinitializer
+  store <4 x i64> %add, <4 x i64> addrspace(1)* null, align 32
+  ret void
+}

llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-getelementptr.ll

define <2 x i32 addrspace(1)*> @vector_gep_v2p1_index_v2i64_constant(<2 x i32 addrspace(1)*> %ptr, <2 x i64> %idx) {
   ; CHECK-NEXT:   [[COPY8:%[0-9]+]]:sgpr_64 = COPY $sgpr30_sgpr31
   ; CHECK-NEXT:   [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 1
   ; CHECK-NEXT:   [[C1:%[0-9]+]]:_(s64) = G_CONSTANT i64 2
   ; CHECK-NEXT:   [[BUILD_VECTOR2:%[0-9]+]]:_(<2 x s64>) = G_BUILD_VECTOR [[C]](s64), [[C1]](s64)
   ; CHECK-NEXT:   [[C2:%[0-9]+]]:_(s64) = G_CONSTANT i64 4
   ; CHECK-NEXT:   [[BUILD_VECTOR3:%[0-9]+]]:_(<2 x s64>) = G_BUILD_VECTOR [[C2]](s64), [[C2]](s64)
   ; CHECK-NEXT:   [[C3:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
   ; CHECK-NEXT:   [[BUILD_VECTOR4:%[0-9]+]]:_(<2 x s64>) = G_BUILD_VECTOR [[C2]](s64), [[C3]](s64)
-  ; CHECK-NEXT:   [[PTR_ADD:%[0-9]+]]:_(<2 x p1>) = G_PTR_ADD [[BUILD_VECTOR]], [[BUILD_VECTOR4]](<2 x s64>)
-  ; CHECK-NEXT:   [[COPY9:%[0-9]+]]:_(<2 x p1>) = COPY [[PTR_ADD]](<2 x p1>)
-  ; CHECK-NEXT:   [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32), [[UV2:%[0-9]+]]:_(s32), [[UV3:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[COPY9]](<2 x p1>)
+  ; CHECK-NEXT:   [[COPY9:%[0-9]+]]:_(<2 x s64>) = COPY [[BUILD_VECTOR4]](<2 x s64>)
+  ; CHECK-NEXT:   [[PTR_ADD:%[0-9]+]]:_(<2 x p1>) = G_PTR_ADD [[BUILD_VECTOR]], [[COPY9]](<2 x s64>)
+  ; CHECK-NEXT:   [[COPY10:%[0-9]+]]:_(<2 x p1>) = COPY [[PTR_ADD]](<2 x p1>)
+  ; CHECK-NEXT:   [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32), [[UV2:%[0-9]+]]:_(s32), [[UV3:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[COPY10]](<2 x p1>)
   ; CHECK-NEXT:   $vgpr0 = COPY [[UV]](s32)
   ; CHECK-NEXT:   $vgpr1 = COPY [[UV1]](s32)
   ; CHECK-NEXT:   $vgpr2 = COPY [[UV2]](s32)
   ; CHECK-NEXT:   $vgpr3 = COPY [[UV3]](s32)
-  ; CHECK-NEXT:   [[COPY10:%[0-9]+]]:ccr_sgpr_64 = COPY [[COPY8]]
-  ; CHECK-NEXT:   S_SETPC_B64_return [[COPY10]], implicit $vgpr0, implicit $vgpr1, implicit $vgpr2, implicit $vgpr3
+  ; CHECK-NEXT:   [[COPY11:%[0-9]+]]:ccr_sgpr_64 = COPY [[COPY8]]
+  ; CHECK-NEXT:   S_SETPC_B64_return [[COPY11]], implicit $vgpr0, implicit $vgpr1, implicit $vgpr2, implicit $vgpr3
   %gep = getelementptr i32, <2 x i32 addrspace(1)*> %ptr, <2 x i64> <i64 1, i64 2>
   ret <2 x i32 addrspace(1)*> %gep
 }