[PATCH] D16240: [InstCombine] Simplify a known nonzero incoming value of PHI

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:940
@@ +939,3 @@
+        Value *VA = PN.getIncomingValue(i);
+        if (!isa<ConstantInt>(VA) && VA->hasOneUse() &&
+            isKnownNonZero(VA, DL, 0, AC, &PN, DT))
----------------
junbuml wrote:
> mssimpso wrote:
> > junbuml wrote:
> > > sanjoy wrote:
> > > > Why do you care about `VA` having only one use?
> > > In correctness perspective, we don't have to check VA->hasOneUse(), but I expected the real impact  on performance only when the incoming value become a dead code from this modification. I'm okay with removing this check (VA->hasOneUse()) if we can expect any indirect benefit.
> > I think keeping the one-use check makes sense. If we only expect there to be a benefit if VA becomes dead, we want to avoid increasing compile-time. The only corner case that I can think of is if all of VAs users are PHIs that are compared with zero.
> > The only corner case that I can think of is if all of VAs users are PHIs that are compared with zero.
> 
> This could a possible corner case, but to be simple for now, I may want to handle it as an extension if we need it.
I appreciate that there is a compile time tradeoff here, but reducing
the number of users of `VA` can help if it changes `VA` to have only
one user left (there are optimizations in instcombine predicated on
instructions having only one use).  In general, my opinion is that if
it does not //hurt// performance to do this transform for `VA` with
more than one user, then we should not bail out.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:942
@@ +941,3 @@
+            isKnownNonZero(VA, DL, 0, AC, &PN, DT))
+          PN.setIncomingValue(i, ConstantInt::get(PN.getType(), 1));
+      }
----------------
`isKnownNonZero` will try to prove the predicate for the location you
pass in as `CtxI`.  You're passing in `PN` for `CtxI`, so currently
you won't optimize cases like:

```
  br (%x > 0), label %left, label %right

left:
  br label %merge  ;; == LOC

right:
  br label %merge

merge:
  %p = phi i32 [ %x, %left ], [ %y, %right ]
  %cmp = icmp eq i32 0, %p

```

The optimization won't trigger in this case because `%x` is not
provably non-zero at `%p`.  However, `%x` //is// provably non-zero at
`LOC`, so passing in `LOC` for `CtxI` to `isKnownNonZero` should
return `true`.


http://reviews.llvm.org/D16240