[PATCH] D84941: [JumpThreading] Let ProcessImpliedCondition look into freeze instructions

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:1253
+    // If the branch conditions of BB and CurrentPred is the same freeze
+    // instruction, predecessor's branch condition implies FICond.
+    if (!Implication && FICond && isa<FreezeInst>(PBI->getCondition())) {
----------------
efriedma wrote:
> 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.)
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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84941/new/

https://reviews.llvm.org/D84941