Page MenuHomePhabricator

Support a funclet operand bundle in LowerInvoke
ClosedPublic

Authored by aheejin on Apr 29 2018, 11:58 PM.

Details

Summary

The current LowerInvoke pass cannot handle invoke instructions with a
funclet bundle operand. The order of operands for an invoke instruction
is {call arguments, callee, funclet operand (if any), normal dest,
unwind dest}. The current code assumes there is no funclet operand and
incorrectly includes a funclet operand into call arguments.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Apr 29 2018, 11:58 PM
rnk added inline comments.Apr 30 2018, 11:03 AM
lib/Transforms/Utils/LowerInvoke.cpp
51 ↗(On Diff #144503)

I think this pass needs to preserve operand bundles. You probably want to call II->getOperandBundlesAsDefs(Bundles), and then remove "funclet" bundles.

test/Transforms/Util/lowerinvoke-funclet.ll
19 ↗(On Diff #144503)

You should be able to extend this test with some arbitrary "test" bundle and then check that it comes out after the pass.

aheejin added inline comments.Apr 30 2018, 5:12 PM
lib/Transforms/Utils/LowerInvoke.cpp
51 ↗(On Diff #144503)

Then is this pass supposed to remove funclet operand bundles for call instructions as well?

Sorry, ping :)

@rnk Ping? Thanks for your comments! I understand that I need to preserve other non-funclet operand bundles, and will do that, but what I'm not sure about is, as I said, if we are supposed to remove "funclet" bundles in this pass, should we do the same thing for "funclet" bundles attached to call instructions for consistency? If we should, would it make sense to be a part of "LowerInvoke" pass, the name that sounds like suggesting it only touches invoke instructions?

smeenai added a subscriber: smeenai.May 5 2018, 3:49 PM
rnk added a comment.May 8 2018, 3:18 PM

(sorry, delayed response due to vacatoin)

lib/Transforms/Utils/LowerInvoke.cpp
51 ↗(On Diff #144503)

I guess it doesn't matter actually. There should be a pass to remove all unreachable code after this pass. I'd recommend transferring all the bundles from the invoke to the call, just for simplicity.

aheejin updated this revision to Diff 145824.May 8 2018, 5:28 PM
  • Preserve operand bundles
aheejin updated this revision to Diff 145825.May 8 2018, 5:31 PM
  • delete a blank line
aheejin updated this revision to Diff 145826.May 8 2018, 5:32 PM
  • delete blank line again
rnk accepted this revision.May 8 2018, 5:38 PM

lgtm!

This revision is now accepted and ready to land.May 8 2018, 5:38 PM
aheejin marked an inline comment as done.May 8 2018, 5:39 PM
aheejin marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.