[IR] Change the default value of InstertElement to poison (1/4)

[hyeongyukim]

Summary

This patch is for fixing potential insertElement-related bugs like D93818.

V = UndefValue::get(VecTy);
for(...)
  V = Builder.CreateInsertElementy(V, Elt, Idx);
=>
V = PoisonValue::get(VecTy);
for(...)
  V = Builder.CreateInsertElementy(V, Elt, Idx);

Like above, this patch changes the placeholder V to poison.
The patch will be separated into several commits.

[hyeongyukim]

Like D110226, D110227, and D110230, It would be better to change undef to poison in one place.

Value *CreateInsertElement(Type *VecTy, Value *NewElt, Value *Idx,
                            const Twine &Name = "") {
   return CreateInsertElement(PoisonValue::get(VecTy), NewElt, Idx, Name);
 }

 Value *CreateInsertElement(Type *VecTy, Value *NewElt, uint64_t Idx,
                            const Twine &Name = "") {
   return CreateInsertElement(PoisonValue::get(VecTy), NewElt, Idx, Name);
 }

@spatel How about adding these two functions, which makes an empty placeholder for InsertElement?

[aqjune]

I left comments on a few changes - I think it is okay to leave ones that are clearly valid if you prefer.

Q: Is there any test that is affected by this?

In D110311#3017248, @hyeongyukim wrote:

Like D110226, D110227, and D110230, It would be better to change undef to poison in one place.

Value *CreateInsertElement(Type *VecTy, Value *NewElt, Value *Idx,
                            const Twine &Name = "") {
   return CreateInsertElement(PoisonValue::get(VecTy), NewElt, Idx, Name);
 }

 Value *CreateInsertElement(Type *VecTy, Value *NewElt, uint64_t Idx,
                            const Twine &Name = "") {
   return CreateInsertElement(PoisonValue::get(VecTy), NewElt, Idx, Name);
 }

I have no preference between adding new ones or explicitly using poison.

llvm/lib/IR/AutoUpgrade.cpp

2420

This change is fine because the for loop below fully hides this value, am I correct?

llvm/lib/Target/NVPTX/NVPTXGenericToNVVM.cpp

215

Is this poison always hidden behind the newly created insertelements below?
In other words, is it possible for this patch to cause a transformation like
<undef, 1, 2> => <poison, mapped(1), mapped(2)>?

llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp

136 ↗

(On Diff #374452)

I'm not sure about the validity of this change; could you explain the context a bit, please?

[hyeongyukim]

Q: Is there any test that is affected by this?

No, tests were not changed by this patch.

This patch is changing the InsertElement's placeholder to poison without changing the LLVM's behavior.
Detailed information on why InsertElement's placeholder should be changed from noundef to poison can be found at https://bugs.llvm.org/show_bug.cgi?id=44185.

Even though this patch is not changing LLVM's behavior, I expect the vector's initialization will be more consistent.

llvm/lib/IR/AutoUpgrade.cpp

2420

Right, other values are always cover the poisons.
Rep is declared as <Elnum x EltTy> poison, and for loop is filling Rep with a new value from the 0th position to the EltNum-1th position. Therefore, all poison in Rep is changed to different values.
Since noundef has not been created before, this patch neither changes the noundef to poison nor makes a new poison.

llvm/lib/Target/NVPTX/NVPTXGenericToNVVM.cpp

215

Whether C is a ConstantVector or a ConstantAggregate, the noundef is not changed to poison.
Like the function UpgradeIntrinsicCall above, all poisons in NewValue are replaced with NewOperand since the for loop is growing 0 to OperandNum-1. So all poisons of NewValue will be removed.
Also, since all NewOperandes are made through recursive function calls, undef is not changed to poison even inside NewOperands.

Even in this case, there is no change in the operation before and after the patch.

llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp

136 ↗

(On Diff #374452)

I can't figure out the exact context either.
It is hard to ensure that there are no changes in LLVM before and after this patch, I will revert this part.

[hyeongyukim]

Revert PPCTargetTransformInfo.cpp

[spatel]

In D110311#3017248, @hyeongyukim wrote:

Like D110226, D110227, and D110230, It would be better to change undef to poison in one place.

Value *CreateInsertElement(Type *VecTy, Value *NewElt, Value *Idx,
                            const Twine &Name = "") {
   return CreateInsertElement(PoisonValue::get(VecTy), NewElt, Idx, Name);
 }

 Value *CreateInsertElement(Type *VecTy, Value *NewElt, uint64_t Idx,
                            const Twine &Name = "") {
   return CreateInsertElement(PoisonValue::get(VecTy), NewElt, Idx, Name);
 }

@spatel How about adding these two functions, which makes an empty placeholder for InsertElement?

Sure - that lines up with the similar create/constructors that we added for shuffles, so it makes things symmetric (even if we don't have too many uses for it currently).
@aqjune - sorry for losing track of D93817 / D94380 . If the newer patches are as you expected, feel free to approve (no need to wait for me).

[aqjune]

@spatel Thanks!
@hyeongyukim Do you mind if we add the new CreateInsertElements and use them in this patch?

[hyeongyukim]

In D110311#3026282, @aqjune wrote:

@spatel Thanks!
@hyeongyukim Do you mind if we add the new CreateInsertElements and use them in this patch?

Of course not, I'll add them. Thanks!

[hyeongyukim]

Add the new CreateInsertElement

[hyeongyukim]

reformat code

[aqjune]

LGTM

llvm/include/llvm/IR/IRBuilder.h

Value *CreateExtractElement(Value *Vec, Value *Idx,
     return Insert(ExtractElementInst::Create(Vec, Idx), Name);
   }
 
   Value *CreateExtractElement(Value *Vec, uint64_t Idx,
                               const Twine &Name = "") {
     return CreateExtractElement(Vec, getInt64(Idx), Name);
   }
 
+  Value *CreateInsertElement(Type *VecTy, Value *NewElt, Value *Idx,
+                              const Twine &Name = "") {
+    return CreateInsertElement(PoisonValue::get(VecTy), NewElt, Idx, Name);
+  }
+
+  Value *CreateInsertElement(Type *VecTy, Value *NewElt, uint64_t Idx,
+                              const Twine &Name = "") {
+    return CreateInsertElement(PoisonValue::get(VecTy), NewElt, Idx, Name);
+  }
+
   Value *CreateInsertElement(Value *Vec, Value *NewElt, Value *Idx,
                              const Twine &Name = "") {
     if (auto *VC = dyn_cast<Constant>(Vec))
       if (auto *NC = dyn_cast<Constant>(NewElt))
         if (auto *IC = dyn_cast<Constant>(Idx))
           return Insert(Folder.CreateInsertElement(VC, NC, IC), Name);
     return Insert(InsertElementInst::Create(Vec, NewElt, Idx), Name);
   }

llvm/lib/IR/AutoUpgrade.cpp

if (IsX86 && (Name.startswith("sse2.pcmp") ||
       // Replace broadcasts with a series of insertelements.
       auto *VecTy = cast<FixedVectorType>(CI->getType());
       Type *EltTy = VecTy->getElementType();
       unsigned EltNum = VecTy->getNumElements();
       Value *Cast = Builder.CreateBitCast(CI->getArgOperand(0),
                                           EltTy->getPointerTo());
       Value *Load = Builder.CreateLoad(EltTy, Cast);
       Type *I32Ty = Type::getInt32Ty(C);
-      Rep = UndefValue::get(VecTy);
+      Rep = PoisonValue::get(VecTy);
       for (unsigned I = 0; I < EltNum; ++I)
         Rep = Builder.CreateInsertElement(Rep, Load,
                                           ConstantInt::get(I32Ty, I));
     } else if (IsX86 && (Name.startswith("sse41.pmovsx") ||
                          Name.startswith("sse41.pmovzx") ||
                          Name.startswith("avx2.pmovsx") ||
                          Name.startswith("avx2.pmovzx") ||
                          Name.startswith("avx512.mask.pmovsx") ||

llvm/lib/Target/NVPTX/NVPTXGenericToNVVM.cpp

Value *GenericToNVVM::remapConstantVectorOrConstantAggregate(
   // If none of the elements has been modified, return C as it is.
   if (!OperandChanged) {
     return C;
   }
 
   // If any of the elements has been  modified, construct the equivalent
   // vector or aggregate value with a set instructions and the converted
   // elements.
-  Value *NewValue = UndefValue::get(C->getType());
+  Value *NewValue = PoisonValue::get(C->getType());
   if (isa<ConstantVector>(C)) {
     for (unsigned i = 0; i < NumOperands; ++i) {
       Value *Idx = ConstantInt::get(Type::getInt32Ty(M->getContext()), i);
       NewValue = Builder.CreateInsertElement(NewValue, NewOperands[i], Idx);
     }
   } else {
     for (unsigned i = 0; i < NumOperands; ++i) {
       NewValue =

llvm/unittests/IR/IRBuilderTest.cpp

 }
 
 TEST_F(IRBuilderTest, InsertExtractElement) {
   IRBuilder<> Builder(BB);
 
   auto VecTy = FixedVectorType::get(Builder.getInt64Ty(), 4);
   auto Elt1 = Builder.getInt64(-1);
   auto Elt2 = Builder.getInt64(-2);
-  Value *Vec = UndefValue::get(VecTy);
-  Vec = Builder.CreateInsertElement(Vec, Elt1, Builder.getInt8(1));
+  Value* Vec = Builder.CreateInsertElement(VecTy, Elt1, Builder.getInt8(1));
   Vec = Builder.CreateInsertElement(Vec, Elt2, 2);
   auto X1 = Builder.CreateExtractElement(Vec, 1);
   auto X2 = Builder.CreateExtractElement(Vec, Builder.getInt32(2));
   EXPECT_EQ(Elt1, X1);
   EXPECT_EQ(Elt2, X2);
 }
 
 TEST_F(IRBuilderTest, CreateGlobalStringPtr) {

llvm/unittests/IR/PatternMatch.cpp

TEST_F(PatternMatchTest, VectorOps) {
   Value *Val = IRB.CreateAdd(IRB.getInt8(0), IRB.getInt8(1));
   Value *Val2 = IRB.CreateAdd(Val, IRB.getInt8(3));
 
   SmallVector<Constant *, 2> VecElemIdxs;
   VecElemIdxs.push_back(ConstantInt::get(i32, 0));
   VecElemIdxs.push_back(ConstantInt::get(i32, 2));
   auto *IdxVec = ConstantVector::get(VecElemIdxs);
 
-  Value *UndefVec = UndefValue::get(VecTy);
-  Value *VI1 = IRB.CreateInsertElement(UndefVec, IRB.getInt8(1), (uint64_t)0);
+  Value *VI1 = IRB.CreateInsertElement(VecTy, IRB.getInt8(1), (uint64_t)0);
   Value *VI2 = IRB.CreateInsertElement(VI1, Val2, Val);
   Value *VI3 = IRB.CreateInsertElement(VI1, Val2, (uint64_t)1);
   Value *VI4 = IRB.CreateInsertElement(VI1, IRB.getInt8(2), Val);
 
   Value *EX1 = IRB.CreateExtractElement(VI4, Val);
   Value *EX2 = IRB.CreateExtractElement(VI4, (uint64_t)0);
   Value *EX3 = IRB.CreateExtractElement(IdxVec, (uint64_t)1);