[PATCH] [InstCombine] call SimplifyICmpInst with correct context

[Jingyue Wu]

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