This is an archive of the discontinued LLVM Phabricator instance.

[GVN] Prevent ScalarPRE from hoisting across instructions that don't pass control flow to successors
ClosedPublic

Authored by mkazantsev on Oct 6 2017, 4:50 AM.

Details

Summary

This is to address a problem similar to those in D37460 for Scalar PRE. We should not
PRE across an instruction that may not pass execution to its successor unless it is safe
to speculatively execute it.

Diff Detail

Event Timeline

This revision is now accepted and ready to land.Nov 3 2017, 11:16 AM
efriedma requested changes to this revision.Nov 3 2017, 11:22 AM
efriedma added inline comments.
lib/Transforms/Scalar/GVN.cpp
2214

Err, wait, sorry, I wasn't reading carefully enough.

What are you trying to check for here? I can't see why it matters if "P" has an implicit control flow instruction; we insert the new instruction at the end of the predecessor, so implicit control flow isn't a problem unless the terminator is an invoke.

This revision now requires changes to proceed.Nov 3 2017, 11:22 AM
mkazantsev added inline comments.Nov 8 2017, 9:26 PM
lib/Transforms/Scalar/GVN.cpp
2214

You're right, I misinterpreted this part. Invoke is also not a problem because if the terminator was an invoke, then we have a critical edge, and we don't PRE across critical edges without splitting them. This check is obsolete, I'll remove it.

mkazantsev added inline comments.Nov 8 2017, 9:31 PM
lib/Transforms/Scalar/GVN.cpp
2214

It also seems that it's OK to do it if we don't need to insert a new instruction, i.e. when NumWithout == 0.

mkazantsev planned changes to this revision.Nov 8 2017, 9:37 PM
mkazantsev edited edge metadata.
mkazantsev marked an inline comment as done.
  1. Removed over-restrictive check;
  2. Allowed PRE across implicit control flow if it only takes to insert a Phi from existing instructons and not create new instructions.
  3. Added a new test against item 2.
efriedma added inline comments.Nov 9 2017, 11:25 AM
lib/Transforms/Scalar/GVN.cpp
2261

"prevent us from that" isn't correct grammar. And you misspelled "implicit".

Maybe move the comment inside the if statement?

mkazantsev marked an inline comment as done.

@efriedma , can I have it landed? :)

This revision is now accepted and ready to land.Nov 27 2017, 2:53 PM
This revision was automatically updated to reflect the committed changes.