[PATCH] [InstCombine] call SimplifyICmpInst with correct context

[Hal Finkel]

----- 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