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

[Sanjay Patel via llvm-commits]

IMO - yes. We're relying on an implementation detail of computeKnownBits
when deciding how to call it. If consistency is the goal, then we should
always pass in the CxtI from InstCombine because we obviously have it.

On Fri, May 26, 2017 at 12:20 PM, Craig Topper <craig.topper at gmail.com>
wrote:

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