[PATCH] [InstCombine] call SimplifyICmpInst with correct context

[Jingyue Wu]

Agreed. Let me see how the correctness issue can be fixed.

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/
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150625/d03db09e/attachment.html>