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

[Peter Collingbourne via llvm-commits]

pcc 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;
----------------
eugenis wrote:
> 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.
Aren't you achieving that by appending the ".unsafe" suffix?

================
Comment at: lib/CodeGen/SafeStack.cpp:637
@@ +636,3 @@
+    // isUsedByMetadata().
+    if ((AI->hasValueHandle() || AI->isUsedByMetadata()) && Replacement) {
+      AI->replaceAllUsesWith(Replacement);
----------------
eugenis wrote:
> 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).
> 
> 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.

That's surprising.
http://llvm-cs.pcc.me.uk/lib/Transforms/Utils/Local.cpp#1223
http://llvm-cs.pcc.me.uk/lib/Transforms/Utils/Local.cpp#1185
It certainly looks like the intent is to replace the address. Is there some case where we aren't replacing it correctly?

> 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).

We shouldn't cargo cult that logic from Value::replaceAllUsesWith, as it only makes sense when the replacement is uniform.


Repository:
  rL LLVM

http://reviews.llvm.org/D20547