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

[Ayman Musa via Phabricator via llvm-commits]

aymanmus added a comment.

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?

Thanks,
Ayman


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

https://reviews.llvm.org/D74484