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

[Sanjay Patel via llvm-commits]

Aha - so I think that answers my initial question: we only pass the context
inst in instcombine from visitICmp/visitFCmp because those are the only
ones that can benefit.

On Fri, May 26, 2017 at 12:06 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>
> 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/0fd6f59f/attachment.html>