[PATCH] D29378: [InstCombine] Allow InstCombine to merge adjacent guards

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:3269
+
+      // Otherwise we try to widen the next guard and remove the current.
+      Value *A, *B, *NewCond;
----------------
sanjoy wrote:
> There's a subtle problem here -- we can transform
> 
> ```
> guard(a) [ "deopt"(X) ]
> guard(b) [ "deopt"(Y) ]
> ```
> 
> to
> 
> ```
> ;; Widen the first guard
> guard(a & b) [ "deopt"(X) ]
> ```
> 
> but not to
> 
> ```
> ;; Widen the second guard
> guard(a & b) [ "deopt"(Y) ]
> ```
> 
> since the deopt state of the second guard (represented as `"Y"` here
> for simplicity) may have been optimized under the assumption that `a`
> is true.
> 
> For instance, say we started with
> 
> ```
> guard(a) [ "deopt"(a) ]
> guard(b) [ "deopt"(a) ]
> ```
> 
> which first got optimized to
> 
> ```
> guard(a) [ "deopt"(a) ]
> guard(b) [ "deopt"(true) ]
> ```
> 
> and then due to this change is optimized to
> 
> ```
> ;; guard(a) [ "deopt"(a) ]
> guard(a && b) [ "deopt"(true) ]
> ```
> 
> Now if `a` is false at runtime then the expected behavior was to
> deoptimize with the deopt state `(a)` == `(false)`, but in the
> optimized program we will deoptimize with the deopt state `(true)`.
> 
> The right transform here is to widen the first guard, not the second
> one.
> 
Thanks for pointing that out: wasn't obvious indeed.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:3273
+          match(NextCond, m_Not(m_Value(B))))
+        // Canonicalize guard(!a); guard(!b) -> guard(!(a | b)).
+        NewCond = Builder->CreateNot(Builder->CreateOr(A, B));
----------------
sanjoy wrote:
> I'd not bother with the `!a && !b` => `!(a && b)`.  I think it is better to leave it as `!a && !b` (i.e. not special case `m_Not` at all), and have InstCombine re-canonicalize it (in a later iteration) to `!(a || b)` if that's more canonical.
Yeah, it is actually done by DeMorgan's Law transformation. :)


https://reviews.llvm.org/D29378