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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

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;

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?


Repository:
  rL LLVM

https://reviews.llvm.org/D52001