[PATCH] D52001: [InstCombine] Inefficient pattern for high-bits checking 2 (PR38708)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D52001#1233768, @spatel wrote:

> I was wondering if it would be easier to read if we swapped everything as a first step, so something like this in the existing code:
>
>   // We want X to be the icmp's second operand, so swap if not.
>   Value *Cmp0 = Cmp.getOperand(0), *X = Cmp.getOperand(1);
>   ICmpInst::Predicate Pred = Cmp.getPredicate(), NewPred;
>   if (match(X, m_OneUse(m_Shl(m_One(), m_Value())))) {
>     std::swap(Cmp0, X);
>     Pred = Cmp.getSwappedPredicate();
>   }
>   Value *Y;
>   if (!match(Cmp0, m_OneUse(m_Shl(m_One(), m_Value(Y)))))
>     return nullptr;
>   if (Pred == ICmpInst::ICMP_ULE) NewPred = ICmpInst::ICMP_NE;
>   else if (Pred == ICmpInst::ICMP_UGT) NewPred = ICmpInst::ICMP_EQ;
>   else return nullptr;
>   
>   


I'm not sure.
I think we really should have two separate matches, and two `switch()`es.
Else i think we may use the wrong predicate..
Also, i *think* i will add one/two patterns to this new matcher (less canonical variants with extra uses),
so specifying the pattern twice seems sub-par.

> It's mostly a matter of taste, but there's a subtle logic difference: what if both sides of the icmp match the shift pattern?
> 
>   define i1 @p0(i8 %shamt0, i8 %shamt1) {
>     %t0 = shl i8 1, %shamt0
>     %t1 = shl i8 1, %shamt1
>     %r = icmp ugt i8 %t0, %t1
>     ret i1 %r
>   }
>   
> 
> 
> With the current code, it gets transformed to:
> 
>   define i1 @p0(i8 %shamt0, i8 %shamt1) {
>     %t1 = shl i8 1, %shamt1
>     %t1.highbits = lshr i8 %t1, %shamt0
>     %r = icmp eq i8 %t1.highbits, 0
>     ret i1 %r
>   }
>   
> 
> 
> It should be reduced:
> 
>   %t0 = shl i8 1, %shamt0
>   %t1 = shl i8 1, %shamt1
>   %r = icmp ugt i8 %t0, %t1
>   =>
>   %r = icmp ugt i8 %shamt0, %shamt1
>    
> 
> I don't think we need to hold up this patch for that, but maybe it changes the way we want to implement it?

I acknowledge that there is some problem when we have the same/similar pattern on the both sides,
i have thought about it a bit (https://reviews.llvm.org/rL342076), but i don't have anything concrete on that.


Repository:
  rL LLVM

https://reviews.llvm.org/D52001