[PATCH] [InstCombine] call SimplifyICmpInst with correct context

[David Majnemer]

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.


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