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

[Jun Bum Lim via llvm-commits]

junbuml 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))
----------------
sanjoy wrote:
> 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.
Looks like FoldOpIntoPhi() also continue trying to fold when a PHI has one non-constant incoming value.  So, I removed VA->hasOneUse() check here.

================
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));
+      }
----------------
sanjoy wrote:
> `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`.
Thanks Sanjoy for the detail. Instead of PN, I pass the terminator of incoming block as CtxI.

================
Comment at: test/Transforms/InstCombine/phi.ll:763-764
@@ -762,1 +762,4 @@
 
+; CHECK-LABEL: @phi_knownnonzero
+; CHECK-NOT: select
+define i1 @phi_knownnonzero(i32 %n, i32 %s, i32* nocapture readonly %P) {
----------------
majnemer wrote:
> This is not a sufficient test.  Please make the test highlight the instruction which you have altered.

I added check for modified PHI.
Thanks David for the comment.


http://reviews.llvm.org/D16240