Page MenuHomePhabricator

[WinEH] Fix inttoptr+phi optimization in presence of catchswitch
ClosedPublic

Authored by rnk on Feb 28 2020, 1:07 PM.

Details

Summary

getFirstInsertionPt's return value must be checked for validity before
casting it to Instruction*. Don't attempt to insert casts after a phi in
a catchswitch block.

Fixes PR45033, introduced in D37832.

Diff Detail

Event Timeline

rnk created this revision.Feb 28 2020, 1:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2020, 1:07 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
hfinkel added inline comments.Feb 28 2020, 2:09 PM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
222

When you say, "a block with no insertion point", I'd add, "for example, a catchswitch block." I don't think that it's otherwise obvious how one has a block with no insertion point.

rnk marked an inline comment as done.Feb 28 2020, 2:23 PM
rnk added inline comments.
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
222

Sure, will do.


I think when we added catchswitch, part of what we were thinking was that getFirstInsertionPt returns an iterator, and an end iterator is a valid iterator, so it wouldn't be unreasonable to expect callers to check them for validity. In practice, people expect the function to do what it says and return a suitable insertion point, and not a failure value.

At the time we considered adding a multi-unwind-destination version of invoke, but I considered it to be too radical. Instead we settled for this unwind-edge-multiplex instruction, catchswitch. Now we have callbr though, so we should rethink this sharp edge of our WinEH (wasm uses it too, I guess) representation.

rnk updated this revision to Diff 247368.Feb 28 2020, 2:26 PM
  • elaborate
davidxl accepted this revision.Feb 28 2020, 4:30 PM

looks ok to me, but wait for Hal to have a final say.

This revision is now accepted and ready to land.Feb 28 2020, 4:30 PM
hfinkel accepted this revision.Feb 28 2020, 6:54 PM

looks ok to me, but wait for Hal to have a final say.

LGTM

This revision was automatically updated to reflect the committed changes.