[PATCH] D125485: [ArgPromotion] Unify byval promotion with non-byval

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:402
+        NewLI->takeName(LI);
+        LI->replaceAllUsesWith(NewLI);
         DeadInsts.push_back(LI);
----------------
psamolysov wrote:
> nikic wrote:
> > Just realized that we can probably just do a `LI->setOperand(0, GetAlloca(Ptr))` here and don't really need to create a new instruction and RAUW.
> Replaced with `LI->setOperand(LoadInst::getPointerOperandIndex(), GetAlloca(Ptr))` Thank you very much for the suggestion. I've applied it for `store` instructions too.
> 
> One thing: when the `LI` was replaced with `OffsetToArg[Offset.getSExtValue()]` in the old code or with the `NewLI` in the patch, the instruction's metadata was not copied. Currently, when we are actually don't replace the instruction, the metadata is reused and in the `metadata.ll` test, the following code is generated:
> 
> ```
> %1 = icmp ne i32* %p2.0.val, null
> call void @llvm.assume(i1 %1)
> ```
> 
> for this line of code:
> 
> ```
>   %v2 = load i32*, i32** %p2, !nonnull !1
> ```
> 
> I've updated the test to take this behavior into account, but is this correct and what is expected?
Yes, this is exactly the effect I wanted to see :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125485