[PATCH] D20547: [safestack] Sink unsafe address computation to each use.

[Evgeniy Stepanov via llvm-commits]

eugenis added inline comments.

================
Comment at: lib/CodeGen/SafeStack.cpp:614
@@ +613,3 @@
+    std::string Name = std::string(AI->getName()) + ".unsafe";
+    AI->setName("");
+    Value *Replacement = nullptr;
----------------
pcc wrote:
> Why bother setting the name if we're about to remove the instruction?
Because otherwise CreateBitCast(..., Name) would de-duplicate the name and the new instruction would end up with something like Name.unsafe.12345.

================
Comment at: lib/CodeGen/SafeStack.cpp:637
@@ +636,3 @@
+    // isUsedByMetadata().
+    if ((AI->hasValueHandle() || AI->isUsedByMetadata()) && Replacement) {
+      AI->replaceAllUsesWith(Replacement);
----------------
pcc wrote:
> This seems a little weird to me. I wouldn't expect anything to be holding a value handle here (and if it did, it probably wouldn't want the replacement for whichever instruction happened to be at the end of the use list) and the only important metadata is debug info, which is handled by `replaceDbgDeclareForAlloca`, I believe. Can we just remove this part?
The new debug metadata generated in replaceDbgDeclareForAlloca is still tied to the alloca that is being replaced. Without this line, it gets replaced with an empty metadata.

The condition (AI->hasValueHandle() || AI->isUsedByMetadata()) is taken from Value::replaceAllUsesWith, and it describes all cases where that function would do any work that is not yet done at this point (i.e. replacing the non-IR uses).



Repository:
  rL LLVM

http://reviews.llvm.org/D20547