[PATCH] D146702: [MergeICmps] Attach metadata to new created loads

[Allen zhong via Phabricator via llvm-commits]

Allen marked an inline comment as done.
Allen added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:646
+    LoadInst *const RhsLoad =
         Builder.CreateLoad(FirstCmp.Rhs().LoadI->getType(), Rhs);
+    // Copy metadata
----------------
Allen wrote:
> nikic wrote:
> > Maybe it would be better to clone the instructions, similar to the GEPs above?
> Thanks for your suggestion. 
> I tried with following change, and find the case  **Transforms/MergeICmps/X86/entry-block-shuffled.ll** will fail.
> this is because we only clone the load instrunction itself, and its PointerOperand will be deleted with following **DeleteDeadBlocks(DeadBlocks, &DTU)** in function BCECmpChain::simplify, then **%4 = load i32, ptr %first.i, align 4** will become  invalid instruction **%4 = load i32, ptr poison, align 4**. Did I miss something ?
> ```
> @@ -639,10 +640,9 @@ static BasicBlock *mergeComparisons(ArrayRef<BCECmpBlock> Comparisons,
>  
>    if (Comparisons.size() == 1) {
>      LLVM_DEBUG(dbgs() << "Only one comparison, updating branches\n");
> -    Value *const LhsLoad =
> -        Builder.CreateLoad(FirstCmp.Lhs().LoadI->getType(), Lhs);
> -    Value *const RhsLoad =
> -        Builder.CreateLoad(FirstCmp.Rhs().LoadI->getType(), Rhs);
> +    Instruction *const LhsLoad = Builder.Insert(FirstCmp.Lhs().LoadI->clone());
> +    Instruction *const RhsLoad = Builder.Insert(FirstCmp.Rhs().LoadI->clone());
> ```
I use replaceUsesOfWith to update the **PointerOperand**, then clone according your comment works fine.


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

https://reviews.llvm.org/D146702