[PATCH] D45108: [InstCombine] Get rid of select of bittest (PR36950 / PR17564)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D45108#1057766, @spatel wrote:

> In https://reviews.llvm.org/D45108#1057522, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D45065#1054271, @spatel wrote:
> >
> > > I think we should solve this using something like https://reviews.llvm.org/D45108 instead, but I need to look closer at how the case in the bug report escaped the matching that we already have in foldSelectInstWithICmp() and foldSelectICmpAnd().
> >
> >
> > Ok, so let's look at those functions
> >
> > - `foldSelectICmpAnd()` is only called when the select has constants as both true and false values https://github.com/llvm-mirror/llvm/blob/0b1a72a7a6069b674f1859ec859e846e6dc63594/lib/Transforms/InstCombine/InstCombineSelect.cpp#L794-L800 This isn't the case in these tests, only the false-value is constant.
> > - Same with the fold before that call https://github.com/llvm-mirror/llvm/blob/0b1a72a7a6069b674f1859ec859e846e6dc63594/lib/Transforms/InstCombine/InstCombineSelect.cpp#L757-L791 Only works on constants.
> >
> >   So while i don't have any particular expirience with InstCombine, i don't see anything that would suggest that `foldSelectInstWithICmp()` is already supposed to handle that case. Am i wrong?
> >
> >   But yes, i should at the very least move the code into that function.
>
>
> Ok, if there's no easy way to meld these together, then just move it over.




> I wasn't sure; just seemed like there was similarity with the existing code.

I agree that the code is *similar*.
But right now i'm not seeing **enough** similarities yet to combine them.

> A couple of things to change while doing that:
> 
> 1. We should separate the case with a shift into its own block. Creating a shift-by-zero and then deleting it isn't efficient. Code readability will also improve.

Ok, sure.

> 2. We need to check 'm_OneUse' to make sure we're not creating more instructions than we're removing. There should also be some negative tests to exercise that. Example:
> 
> 
> 
>   declare void @use32(i32)
>   declare void @use1(i1)
>   
>   define i32 @f_var1(i32 %x, i32 %y) {
>     %t3 = and i32 %x, %y
>     %t4 = icmp eq i32 %t3, 0
>     %t5 = and i32 %x, 1
>     %t6 = select i1 %t4, i32 %t5, i32 1
>     call void @use32(i32 %t3)
>     call void @use32(i32 %t5)
>     call void @use1(i1 %t4)
>     ret i32 %t6
>   }
>   
> 
> 
> We shouldn't have 3 extra instructions here.

Thank you for the test! I was wondering about that (in the differential's description)


Repository:
  rL LLVM

https://reviews.llvm.org/D45108