[PATCH] [InstCombine] call SimplifyICmpInst with correct context

[Jingyue Wu]

Never mind, I didn't see Hal LGTM'ed it already. I'll go ahead submit it
soon.

On Thu, Jun 25, 2015 at 12:57 PM, Jingyue Wu <jingyue at google.com> wrote:

> Sounds good. Thanks for the explanation, Hal.
>
> So, do you think this patch is good to go then?
>
> On Thu, Jun 25, 2015 at 11:43 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>
>> ----- Original Message -----
>> > From: "David Majnemer" <david.majnemer at gmail.com>
>> > To: "Hal Finkel" <hfinkel at anl.gov>
>> > Cc: "Jingyue Wu" <jingyue at google.com>,
>> reviews+D10695+public+1cd92e3c8232918a at reviews.llvm.org, "henry hu sh"
>> > <henry.hu.sh at gmail.com>, "LLVM Commits" <llvm-commits at cs.uiuc.edu>,
>> "Xuetian Weng" <xweng at google.com>, "Bjarke
>> > Roune" <broune at google.com>, "Mark Heffernan" <meheff at google.com>, "Eli
>> Bendersky" <eliben at google.com>
>> > Sent: Thursday, June 25, 2015 1:34:27 PM
>> > Subject: Re: [PATCH] [InstCombine] call SimplifyICmpInst with correct
>> context
>> >
>> >
>> >
>> >
>> >
>> >
>> > On Thu, Jun 25, 2015 at 11:29 AM, David Majnemer <
>> > david.majnemer at gmail.com > wrote:
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> > On Thu, Jun 25, 2015 at 11:22 AM, Hal Finkel < hfinkel at anl.gov >
>> > wrote:
>> >
>> >
>> > ----- Original Message -----
>> > > From: "Jingyue Wu" < jingyue at google.com >
>> > > To: "David Majnemer" < david.majnemer at gmail.com >
>> > > Cc: reviews+D10695+public+1cd92e3c8232918a at reviews.llvm.org , "Hal
>> > > Finkel" < hfinkel at anl.gov >, "henry hu sh"
>> > > < henry.hu.sh at gmail.com >, "LLVM Commits" <
>> > > llvm-commits at cs.uiuc.edu >, "Xuetian Weng" < xweng at google.com >,
>> > > "Bjarke
>> > > Roune" < broune at google.com >, "Mark Heffernan" < meheff at google.com
>> > > >, "Eli Bendersky" < eliben at google.com >
>> > > Sent: Thursday, June 25, 2015 12:56:13 PM
>> > > Subject: Re: [PATCH] [InstCombine] call SimplifyICmpInst with
>> > > correct context
>> > >
>> > >
>> > > Agreed. Let me see how the correctness issue can be fixed.
>> > >
>> >
>> > I'm missing something. What's the correctness issue?
>> >
>> >
>> >
>> > My concern is that there is no way to communicate that no assume
>> > context is appropriate for analyzing an instruction.
>> >
>> >
>> > However, maybe this isn't problematic. I couldn't come up with a
>> > plausible scenario where we'd want to run without a context because
>> > doing so might result in miscompiles and the only harm in inferring
>> > a context is that we might nuke an assume earlier than we might've.
>>
>> I'm fairly certain that there isn't one. If there is, then I made a
>> mistake somewhere.
>>
>>  -Hal
>>
>> >
>> >
>> > -Hal
>> >
>> >
>> >
>> > >
>> > > On Wed, Jun 24, 2015 at 10:23 PM, David Majnemer <
>> > > david.majnemer at gmail.com > wrote:
>> > >
>> > >
>> > > On Wed, Jun 24, 2015 at 10:00 PM, Jingyue Wu < jingyue at google.com >
>> > > wrote:
>> > >
>> > >
>> > > And just to be clear, I agree with you that it's a potential issue
>> > > that `ComputeSignBit` is unable to say "I don't have a context".
>> > > But
>> > > my concern is, even if we fix that issue, not passing the context
>> > > instruction to `SimplifyICmpInst` at all in `InstCombine` may
>> > > overkill some useful optimizations.
>> > >
>> > >
>> > >
>> > > I see these as two separable issues: a correctness issue and a
>> > > performance issue. My thinking was that we should solve the
>> > > correctness issue by being conservative at the API boundary and
>> > > regain lost performance by providing the context instruction. My
>> > > fear is not that your change to SimplifyICmpInst is incorrect but
>> > > that all the other callers of the various SimplifyXYZInst API have
>> > > latent bugs because they aren't passing in a context.
>> > >
>> > >
>> > >
>> > > Btw, I noticed that `SimplifyDemandedBits` indeed pass `UserI` as
>> > > the
>> > > context instruction (
>> > >
>> http://llvm.org/docs/doxygen/html/InstCombineSimplifyDemanded_8cpp_source.html#l00075
>> > > ), which seems aligned with the approach in my patch.
>> > >
>> > >
>> > >
>> > >
>> > > http://reviews.llvm.org/D10695
>> > >
>> > > EMAIL PREFERENCES
>> > > http://reviews.llvm.org/settings/panel/emailpreferences/
>> > >
>> > >
>> > >
>> > >
>> > >
>> >
>> > --
>> > Hal Finkel
>> > Assistant Computational Scientist
>> > Leadership Computing Facility
>> > Argonne National Laboratory
>> >
>> >
>> >
>>
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150625/1fcae7be/attachment.html>