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

[Craig Topper via llvm-commits]

But all the calls to computeKnownBits made in non icmp/fcmp code pass the
context inst. Shouldn't we be consistent by giving it to the
commuteKnownBits calls InstSimplify might do as well?

~Craig

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

> 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/36d0954e/attachment.html>