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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:3269
+
+      // Otherwise we try to widen the next guard and remove the current.
+      Value *A, *B, *NewCond;
----------------
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.



================
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));
----------------
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.


https://reviews.llvm.org/D29378