[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:29:11 PM
> Subject: Re: [PATCH] [InstCombine] call SimplifyICmpInst with correct context
> 
> 
> 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.
> 

No, there is always an appropriate context (the instruction itself, which is what is used by default for an instruction if you don't pass another context and the instruction is inserted). Not passing a context might cause an optimization to trivialize the condition (causing the removal of the assumption), but that's not a correctness problem. Like with preserving useful metadata, there is some amount of auditing involved (but better, it always fails in the conservatively-correct way).

 -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