This is an archive of the discontinued LLVM Phabricator instance.

[SCEVExpander] Drop poison generating flags when reusing instructions
ClosedPublic

Authored by reames on Oct 28 2021, 10:07 AM.

Details

Summary

This is a second attempt at the issue from D112637, but hopefully more correct this time. :)

The basic problem we have is that we're trying to reuse an instruction which is mapped to some SCEV. Since we can have multiple such instructions (potentially with different flags), this is analogous to our need to drop flags when performing CSE. A trivial implementation would simply drop flags on any instruction we decided to reuse, and that would be correct.

This patch is almost that trivial patch except that we preserve flags on the reused instruction when existing users would imply UB on overflow already. Adding new users can, at most, refine this program to one which doesn't execute UB which is valid.

In practice, this fixes two conceptual problems with the previous code: 1) a binop could have been canonicalized into a form with different opcode or operands, or 2) the inbounds GEP case which was simply unhandled.

This area of code is a tad more subtle than any of us would like, and my last attempt was definitely unsound. I'd appreciate review with a skeptical eye.

On the test changes, most are pretty straight forward. We loose some flags (in some cases, they'd have been dropped on the next CSE pass anyways). The one that took me the longest to understand was the ashr-expansion test. What's happening there is that we're considering reuse of the mul, previously we disallowed it entirely, now we allow it with no flags. The surrounding diffs are all effects of generating the same mul with a different operand order, and then doing simple DCE.

The loss of the inbounds is unfortunate, but even there, we can recover most of those once we actually treat branch-on-poison as immediate UB.

Diff Detail

Event Timeline

reames created this revision.Oct 28 2021, 10:07 AM
reames requested review of this revision.Oct 28 2021, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2021, 10:07 AM
mkazantsev added inline comments.Nov 2 2021, 1:28 AM
llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
1947

Will we drop flags if both copies had them?

reames added inline comments.Nov 4 2021, 9:37 AM
llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
1947

What do you mean by "both copies"?

mkazantsev added inline comments.Nov 7 2021, 9:35 PM
llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
1947

The comment before this code mentions that we are CSEing two copies of same instruction with potentially different flags, and drop them if we failed to prove legality. But what if both of these copies had the same flags, do we still need to try to prove something and drop the flags? Why?

reames added inline comments.Nov 16 2021, 8:41 AM
llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
1947

Ah. The problem is that one of the two instructions is unknown. It corresponds to the SCEV we're generating, and translating SCEV flags directly to instruction flags is non-trivial. As such, we must use the worst interpretation for the unknown instruction. As such, we're effectively CSEing an instruction with no-flags with whatever instruction we decided to reuse, and thus have to drop flags.

mkazantsev accepted this revision.Nov 28 2021, 9:19 PM

Ok, thanks for clarification.

This revision is now accepted and ready to land.Nov 28 2021, 9:19 PM
This revision was landed with ongoing or failed builds.Nov 29 2021, 3:29 PM
This revision was automatically updated to reflect the committed changes.