[PATCH] D74484: [AggressiveInstCombine] Add support for ICmp instr that feeds a select intsr's condition operand.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D74484#1967040 <https://reviews.llvm.org/D74484#1967040>, @aymanmus wrote:

> In D74484#1966744 <https://reviews.llvm.org/D74484#1966744>, @spatel wrote:
>
> > In D74484#1963795 <https://reviews.llvm.org/D74484#1963795>, @aymanmus wrote:
> >
> > > @spatel: Basically the test cases with the real motivation are the first 4.
> >
> >
> > Do you mean the first 4 with diffs, or the first 4 that are being added as new tests?
>
>
> The first 4 with diff.
>
> > 
> > 
> >> Where most of the other cases are there to check various "edge" cases of the added code behavior.
> >>  I think, that even if some of the tests are not in a canonical form (and can be optimized by instcombine), we still have an added value having them here in order to check the behavior of this specific pass with similar cases.
> >>  Don't you agree?
> > 
> > Yes, I agree that we want to have tests for edge cases to make sure that the logic is correct here. But we also should have tests that show why this patch is necessary - functions that could not be solved in regular instcombine easily.
>
> I agree with you 100%. That's why we have several test cases.
>
> Running the same test with -instcombine instead of -aggressive-instcombine, out of the 23 test cases in the file, the following get optimized:
>
> - cmp_select_zext_i8_noTransformation
> - cmp_select_zext_sext_i8_noTransformation
> - cmp_select_signed_const_i16Const_noTransformation
> - cmp_select_unsigned_const_i16Const
> - cmp_select_unsigned_const_i16Const_noTransformation
> - cmp_select_unsigned_const_i16_MSB1
> - cmp_select_bigConst_cmp
> - cmp_zext_and_minus1_noTransformation
>
>   Out of these cases, the ones with "_noTransformation" suffix are there to make sure our pass does not apply any transformations, and the others include some special immediate values.
>
>   Actually, I feel that I might have misunderstood your comments intention. Can you explain to me what are you suggesting that we do?


I'm trying to understand which patterns are not capable of being reduced to the optimal form by instcombine. If there is some artificial limitation within regular instcombine that we can overcome (like unnecessary bypassing of min/max patterns), then we might be able to avoid a specialized solution in this pass. We want to show that there's a good reason to add code here in aggressive-instcombine to handle these optimizations. So if you can add comments in the test file that explain why instcombine can't handle some transform, that will explain to future readers of the code/tests why we have this particular transform here. Also, if instcombine somehow gets smarter in the future, then we would know if this code became redundant.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74484/new/

https://reviews.llvm.org/D74484