This is an archive of the discontinued LLVM Phabricator instance.

[JumpThreading] Let ProcessImpliedCondition look into freeze instructions
ClosedPublic

Authored by aqjune on Jul 30 2020, 6:33 AM.

Details

Summary

This patch makes JumpThreading's ProcessImpliedCondition deal with frozen
conditions.

Diff Detail

Event Timeline

aqjune created this revision.Jul 30 2020, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2020, 6:33 AM
aqjune requested review of this revision.Jul 30 2020, 6:33 AM
efriedma added inline comments.Jul 30 2020, 12:49 PM
llvm/lib/Transforms/Scalar/JumpThreading.cpp
1253

Would it make sense to improve isImpliedCondition, instead of manipulating the result here? (I haven't looked at the other users of isImpliedCondition, but it seems like the sort of thing isImpliedCondition should handle.)

aqjune added inline comments.Jul 30 2020, 11:56 PM
llvm/lib/Transforms/Scalar/JumpThreading.cpp
1253

Actually, I have to say that it's hard to answer; After isImpliedCondition says something about an expression having freeze, the freeze should be properly consumed. This is needed to prevent other analyses/optimizations from concluding different result from the expression.

For example, let's consider this example:

c = freeze undef
if (c) {
  c2 = freeze undef
  print(c2)
  print(c2) // These two prints should show the same result
}

If two prints are optimized by different analyses/transformations, they may print inconsistent values, which should be avoided.
I looked through other uses of isImpliedCondition, and whether this consistency could be met (by consuming the freeze) wasn't clear to me.

Another issue was that, the semantics of existing analysis functions (computeKnownBits, isImpliedCondition, isTruePredicate, etc) isn't clear when undef/poison is involved. I mean, at least documentation-wise. I believe this is another big thing to be clarified in the future... maybe along with the semantics of attributes like nonnull/align/etc.

aqjune updated this revision to Diff 282151.Jul 31 2020, 1:23 AM

Fix another failing test

nikic accepted this revision.May 17 2022, 4:44 AM

LGTM

llvm/lib/Transforms/Scalar/JumpThreading.cpp
1226

nit: "or a nondeterministic value"

This revision is now accepted and ready to land.May 17 2022, 4:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 4:44 AM
xbolva00 added inline comments.
llvm/lib/Transforms/Scalar/JumpThreading.cpp
1253

Maybe extra parameter for isImpliedCondition to peek thru freezeinst?

nikic added inline comments.May 17 2022, 6:01 AM
llvm/lib/Transforms/Scalar/JumpThreading.cpp
1253

I don't think that's a good idea: An important aspect here is that the freeze is one-use and is dropped as part of the transform. I don't think this can be incorporated into the isImpliedCondition() API in a way that makes sense.

aqjune updated this revision to Diff 430214.May 17 2022, 6:28 PM

Make test show diff, add a description about why @and_noopt is not optimized yet, update comments in ProcessImpliedCondition

This revision was landed with ongoing or failed builds.May 17 2022, 6:51 PM
This revision was automatically updated to reflect the committed changes.