This is an archive of the discontinued LLVM Phabricator instance.

Replace llvm.frameallocate with llvm.frameescape
ClosedPublic

Authored by rnk on Mar 3 2015, 6:47 PM.

Details

Summary

Turns out it's pretty straightforward and simplifies the implementation.

We need some more cleanups here, but those can come later. I'm trying to send
patches quickly to continuously integrate. :)

Diff Detail

Event Timeline

rnk updated this revision to Diff 21164.Mar 3 2015, 6:47 PM
rnk retitled this revision from to Replace llvm.frameallocate with llvm.frameescape.
rnk updated this object.
rnk added a reviewer: andrew.w.kaylor.
rnk added a subscriber: Unknown Object (MLST).
andrew.w.kaylor added inline comments.Mar 4 2015, 4:01 PM
lib/CodeGen/WinEHPrepare.cpp
293–294

Since we don't need to build a structure type, this loop can be combined with the loop that replaces the temporary allocas.

Also, this comment is now wrong.

293–294

This comment is wrong.

307–308

This comment is wrong.

351

This bitcast won't be necessary if TempAlloca's type is already i8*. Given how later we are in the process, is this something that should be checked here. Similarly, if the TempAlloca value is cast back to an i8* for some reason we should avoid that.

353

This shouldn't be here.

lib/IR/Verifier.cpp
2907

Is there any way we can find the parent's frameescape call and verify that the index is in range?

lib/MC/MCContext.cpp
137

I find the use of "frame_offset" potentially confusing in this name. I prefer something like "frame_escape" that gives some clue as to what the index value means.

test/CodeGen/WinEH/cppeh-frame-vars.ll
179–188

Should %e.i81 here be %e.18? If not, why not?

test/CodeGen/WinEH/cppeh-inalloca.ll
141

This bitcast is redundant, since %e.i81 already has this pointer with an i8* type. It also doesn't seem to be used. Is this an artifact of %e.i8 having been passed to llvm.eh.begincatch in the original code?

test/CodeGen/WinEH/cppeh-min-unwind.ll
27

These values should be eliminated after the outlined handlers are pruned, so it's probably better not to have them checked now.

rnk added inline comments.Mar 4 2015, 5:27 PM
lib/CodeGen/WinEHPrepare.cpp
293–294

Tried to clean this up a bit

293–294

Fixed

307–308

Fixed

351

True.

353

oops

lib/IR/Verifier.cpp
2907

This is kind of awkward because the verifier is a FunctionPass, but we can do it after we've processed the whole module. It took some doing, but it should be in the next patch.

lib/MC/MCContext.cpp
137

Sounds reasonable.

test/CodeGen/WinEH/cppeh-frame-vars.ll
179–188

There's an e.i8 value in the original program, and llvm variable renaming causes us to get "e.i8" + "1". Probably the easiest thing to do is to remove the original e.i8 value, or to cleanup dead instructions.

test/CodeGen/WinEH/cppeh-inalloca.ll
141

Yeah. We could try erasing such dead instructions after outlining a begincatch call.

test/CodeGen/WinEH/cppeh-min-unwind.ll
27

True.

rnk updated this revision to Diff 21248.Mar 4 2015, 5:27 PM
  • Respond to review comments
andrew.w.kaylor accepted this revision.Mar 4 2015, 5:48 PM
andrew.w.kaylor edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Mar 4 2015, 5:48 PM
This revision was automatically updated to reflect the committed changes.
spatel added a subscriber: spatel.Mar 8 2015, 9:09 AM
spatel added inline comments.
llvm/trunk/lib/IR/Verifier.cpp
1279 ↗(On Diff #21287)

This print shouldn't have made it into the commit?

rnk added inline comments.Mar 9 2015, 12:55 PM
llvm/trunk/lib/IR/Verifier.cpp
1279 ↗(On Diff #21287)

Was removed in r231391

test/CodeGen/X86/frameescape.ll