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

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:402
+        NewLI->takeName(LI);
+        LI->replaceAllUsesWith(NewLI);
         DeadInsts.push_back(LI);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125485