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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

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 <https://reviews.llvm.org/D110311#3017248>, @hyeongyukim wrote:

> Like D110226 <https://reviews.llvm.org/D110226>, D110227 <https://reviews.llvm.org/D110227>, and D110230 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:2420
       Type *I32Ty = Type::getInt32Ty(C);
-      Rep = UndefValue::get(VecTy);
       for (unsigned I = 0; I < EltNum; ++I)
----------------
This change is fine because the `for` loop below fully hides this value, am I correct?


================
Comment at: llvm/lib/Target/NVPTX/NVPTXGenericToNVVM.cpp:215
   // elements.
-  Value *NewValue = UndefValue::get(C->getType());
   if (isa<ConstantVector>(C)) {
----------------
Is this poison always hidden behind the newly created `insertelement`s below?
In other words, is it possible for this patch to cause a transformation like
<undef, 1, 2> => <poison, mapped(1), mapped(2)>?


================
Comment at: llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp:136
             IC.Builder.CreateBitCast(II.getArgOperand(1), Mask->getType());
-        Value *Result = UndefValue::get(Op0->getType());
 
----------------
I'm not sure about the validity of this change; could you explain the context a bit, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110311