Page MenuHomePhabricator

[safestack] Sink unsafe address computation to each use.
ClosedPublic

Authored by eugenis on May 23 2016, 5:22 PM.

Details

Reviewers
pcc
Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 58177.May 23 2016, 5:22 PM
eugenis retitled this revision from to [safestack] Sink unsafe address computation to each use..
eugenis updated this object.
eugenis added a reviewer: pcc.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: llvm-commits.
pcc added inline comments.Jun 2 2016, 1:21 PM
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?

eugenis added inline comments.Jun 2 2016, 1:38 PM
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).

pcc added inline comments.Jun 2 2016, 1:48 PM
lib/CodeGen/SafeStack.cpp
615

Aren't you achieving that by appending the ".unsafe" suffix?

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.

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.

eugenis added inline comments.Jun 2 2016, 3:06 PM
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

eugenis updated this revision to Diff 59623.Jun 3 2016, 2:24 PM

Rebased on top of D20986

lib/CodeGen/SafeStack.cpp
615

Removed.

649

Fixed in D20986.

pcc added inline comments.Jun 13 2016, 4:46 PM
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?

eugenis updated this revision to Diff 60763.Jun 14 2016, 2:49 PM
eugenis marked 2 inline comments as done.
pcc added inline comments.Jun 14 2016, 3:00 PM
lib/CodeGen/SafeStack.cpp
613

Is that the right PR number?

eugenis updated this revision to Diff 60769.Jun 14 2016, 3:03 PM

fix PR number in comments

pcc accepted this revision.Jun 14 2016, 3:13 PM
pcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 14 2016, 3:13 PM
eugenis closed this revision.Jun 16 2016, 3:41 PM

r272969