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

[Daniel Berlin via llvm-commits]

On Fri, May 26, 2017 at 10:54 AM, Sanjay Patel <spatel at rotateright.com>
wrote:

>
>
> On Fri, May 26, 2017 at 11:13 AM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
>
>> I tried to make sure we passed the right instruction in each case, but i
>> may have screwed up :)
>> note that:
>>  SQ.getWithInstruction(&I)
>> vs
>>  SQ will give different results if the *context instruction* is not I.
>>
>>
> Is that because we only use CxtI in icmp-related queries like
> computeKnownBitsFromAssume?
>
Yes


> Ie, when we call Simplify*Inst with the plain 'SQ' in instcombine, CxtI is
> 'null' in that query. There is nothing filling that in with the default 'I'
> from what I see.
>
> We fill it in from SimplifyInstruction, but not other things, because we
don't have it :)



> If the simplify routine uses computeKnownBits or some other value
> tracking, then there could be a different result if known bits was somehow
> using CxtI for non-icmp analysis?
>

I believe, at the moment, it will only affect assumes.
At lesat, when i screwed it up, that was the only testcase that failed.


>
>
>
>> I swear there was a case in instcombine where this the case originally,
>> and i made sure it continues.
>>
>> On Fri, May 26, 2017 at 10:06 AM, Sanjay Patel via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> 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
>>>
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170526/9df34dec/attachment.html>