[PATCH] D33567: [InstCombine] Pass the DominatorTree and AssumptionCache to a few calls to isKnownPositive, isKnownNegative, and isKnownNonZero

[Sanjay Patel via Phabricator via llvm-commits]

spatel added subscribers: davide, hfinkel, dberlin.
spatel added a comment.

Can you concoct tests with 'llvm.assume' to show a difference?

I think that raises a question though: this change can make value tracking more effective, but it comes with non-zero compile-time cost if I understand computeKnownBits() correctly. Do we want to incur that overhead without a real-world motivation for the benefit? 
cc'ing @dberlin @davide @hfinkel

Another question along this line: we call Simplify*Inst() for all instructions as the first step in every visit*() in InstCombine. In most cases, we don't pass a context instruction, but we do for visitICmpInst(). Is that difference intentional?

  if (Value *V = SimplifyICmpInst(I.getPredicate(), Op0, Op1,
                                  SQ.getWithInstruction(&I)))
    return replaceInstUsesWith(I, V);

vs.

  if (Value *V = SimplifyShlInst(Op0, Op1, I.hasNoSignedWrap(),
                                  I.hasNoUnsignedWrap(), SQ))
     return replaceInstUsesWith(I, V);


https://reviews.llvm.org/D33567