This is a fix for PR27844.
When replacing uses of unsafe allocas, emit the new location immediately after each use.
Without this, the pointer stays live from the function entry to the last use, while it's usually cheaper to recalculate.
Details
- Reviewers
pcc
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/CodeGen/SafeStack.cpp | ||
---|---|---|
615 | Why bother setting the name if we're about to remove the instruction? | |
638 | 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? |
lib/CodeGen/SafeStack.cpp | ||
---|---|---|
615 | Because otherwise CreateBitCast(..., Name) would de-duplicate the name and the new instruction would end up with something like Name.unsafe.12345. | |
638 | 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). |
lib/CodeGen/SafeStack.cpp | ||
---|---|---|
615 | Aren't you achieving that by appending the ".unsafe" suffix? | |
638 |
That's surprising.
We shouldn't cargo cult that logic from Value::replaceAllUsesWith, as it only makes sense when the replacement is uniform. |
lib/CodeGen/SafeStack.cpp | ||
---|---|---|
615 | Right. Will remove. | |
638 | That case is llvm.dbg.value. replaceDbgDeclareForAlloca does not touch dbg.value. They are updated by RAUW, but that does not give them the same "good" debug location as replaceDbgDeclareForAlloca does for dbg.declare. I'll fix it in replaceDbgDeclareForAlloca |
lib/CodeGen/SafeStack.cpp | ||
---|---|---|
613 | Explain why in the comment. | |
631 | I believe this can happen when a function has multiple identical control flow edges, e.g. define void @main() { entry: br i1 undef, label %x, label %x x: %p = phi i32 [ 0, %entry ], [ 0, %entry ] ret void } Can you please add tests that cover phi nodes, including this case? |
lib/CodeGen/SafeStack.cpp | ||
---|---|---|
613 | Is that the right PR number? |
Explain why in the comment.