Page MenuHomePhabricator

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

Repository
rL LLVM

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
306 ↗(On Diff #21164)

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.

340 ↗(On Diff #21164)

This comment is wrong.

355 ↗(On Diff #21164)

This comment is wrong.

412 ↗(On Diff #21164)

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.

414 ↗(On Diff #21164)

This shouldn't be here.

lib/IR/Verifier.cpp
2883 ↗(On Diff #21164)

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

lib/MC/MCContext.cpp
136 ↗(On Diff #21164)

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 ↗(On Diff #21164)

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

test/CodeGen/WinEH/cppeh-inalloca.ll
141 ↗(On Diff #21164)

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 ↗(On Diff #21164)

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
306 ↗(On Diff #21164)

Tried to clean this up a bit

340 ↗(On Diff #21164)

Fixed

355 ↗(On Diff #21164)

Fixed

412 ↗(On Diff #21164)

True.

414 ↗(On Diff #21164)

oops

lib/IR/Verifier.cpp
2883 ↗(On Diff #21164)

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
136 ↗(On Diff #21164)

Sounds reasonable.

test/CodeGen/WinEH/cppeh-frame-vars.ll
179 ↗(On Diff #21164)

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 ↗(On Diff #21164)

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

test/CodeGen/WinEH/cppeh-min-unwind.ll
27 ↗(On Diff #21164)

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

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

Was removed in r231391