[PATCH] [InstCombine] call SimplifyICmpInst with correct context

[David Majnemer]

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.


>
>
>>
>>  -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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150625/eccbae3f/attachment.html>