[PATCH] D17873: [InstCombine] (icmp sgt smin(PosA, B) 0) -> (icmp sgt B 0)

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3181
@@ +3180,3 @@
+        if (SPR.Flavor == SPF_SMIN) {
+          if (isKnownNonNegative(A, DL) && isKnownNonZero(A, DL))
+            return new ICmpInst(I.getPredicate(), B, CI);
----------------
reames wrote:
> sanjoy wrote:
> > This is more work for you, but I think the right thing to do here is introduce an `isKnownPositive` helper in ValueTracking.h -- doing two recursive walks on the same expression seems wasteful.
> > 
> > I'd say lets start with a simple `isKnownPositive` implementation (maybe one that just looks at `!range` metadata and `computeKnownBits`), which we can evolve in the future.
> > 
> I disagree that introducing yet another independent implementation is a good idea.  
> 
> To be blunt, the extra work to make the approach you're requesting work isn't worth it for this patch.  I'll just abandon the patch instead because it turns out not to address the case I actually care about.
> I disagree that introducing yet another independent implementation is a good idea.

I grepped around in the codebase, and it looks like `SimplifyICmpInst` already has this idiom (i.e. `isKnownNonNegative(X)` followed by `isKnownNonZero(X)`), so I suppose one more instance of it can't hurt.  The avoidable 6-deep traversal still irks me though -- maybe we can leave a FIXME here?


http://reviews.llvm.org/D17873