[PATCH] [InstCombine] call SimplifyICmpInst with correct context

[Hal Finkel]

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

 -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