[LVI] Handle icmp of ashr.

[aemerson]

Summary

This handles the case where this combine:

icmp sgt (ashr X, ShAmtC), C --> icmp sgt X, ((C + 1) << ShAmtC) - 1

wasn't performed by instcombine.

This is my first patch to LVI and I'm not really sure this is right.

Proof of the original combine: https://alive2.llvm.org/ce/z/SfpsvX

[aemerson]

ping

[nikic]

Can you please add an alive2 proof to the patch description?

llvm/lib/Analysis/LazyValueInfo.cpp

1154

For symmetry reasons, this needs to handle all signed predicates (sgt, sge, slt, sle). The non-strict predicates are non-canonical by themselves, but will appear when the inverse predicate is taken.

[aemerson]

The original proof from 7ac2db6a489f3f8e588589a69164882df7973d34 isn't alive (ha) anymore, so I re-created it: https://alive2.llvm.org/ce/z/SfpsvX

[nikic]

In D151911#4416351, @aemerson wrote:

The original proof from 7ac2db6a489f3f8e588589a69164882df7973d34 isn't alive (ha) anymore, so I re-created it: https://alive2.llvm.org/ce/z/SfpsvX

Thanks! However, that proof has a bunch of preconditions -- don't we need to check them here as well?

llvm/lib/Analysis/LazyValueInfo.cpp

1155

Don't we have to adjust the range based on the predicate? E.g. the result for sgt and sle would be inverts of each other, while this returns the same range for both.

[aemerson]

Fix the ranges for the other predicates.

Also add precondition checks. However I wasn't able to construct a test that actually covers the checks because those all get optimized away before the checks are hit.

[aemerson]

ping

[aemerson]

ping @nikic does this look right?

[aemerson]

ping

[nikic]

llvm/lib/Analysis/LazyValueInfo.cpp

1152

I think the better way to explain this is icmp slt (ashr X, ShAmtC), C --> icmp slt X, C << ShAmtC (https://alive2.llvm.org/ce/z/ww5u4i). In this case the only precondition is ((C << ShAmtC) >> ShAmtC) == C. Same for sge.

All the +1/-1 get introduced by using sgt/sle because we're effectively trying to reduce them to that base case. This also introduces the extra preconditions to avoid overflow on +/-1.

With this in mind, I think we can introduce an abstraction like this to transparently handle all the signed predicates while only specifying the fold for slt (without knowledge of anything about ashr in particular):

static std::optional<ConstantRange> getRangeViaSLT(
    CmpInst::Predicate Pred, APInt RHS,
    function_ref<std::optional<ConstantRange>(const APInt &)> Fn) {
  bool Invert = false;
  if (Pred == ICmpInst::SGT || Pred == ICmpInst::SGE) {
    Pred = ICmpInst::getInversePredicate(Pred);
    Invert = true;
  }
  if (Pred == ICmpInst::SLE) {
    Pred = ICmpInst::SLT;
    if (RHS.isSignedMax())
      return std::nullopt; // Could also return full/empty here, if we wanted.
    ++RHS;
  }
  assert(Pred == ICmpInst::SLT && "Must be signed predicate");
  if (auto CR = Fn(RHS))
    return Invert ? CR->inverse() : CR;
  return std::nullopt;
}

And then do something like this:

const APInt *ShAmtC;
if (CmpInst::isSigned(EdgePred) &&
    match(LHS, m_AShr(m_Specific(Val), m_APInt(ShAmtC))) &&
    match(RHS, m_APInt(C))) {
  return getRangeViaSLT(EdgePred, *C, [&](const APInt &RHS) {
    APInt New = RHS << *ShAmtC;
    if ((New.ashr(*ShAmtC)) != New)
      return std::nullopt;
    return ConstantRange::getNonEmpty(APInt:.getSignedMinValue(BW), New + 1);
  });
}

Code is untested, but I think the general idea should work.

llvm/test/Transforms/CorrelatedValuePropagation/icmp.ll

1257

Can we have a test where the RHS is not zero? We should also have one where the shift round trip precondition does not hold.

[aemerson]

llvm/lib/Analysis/LazyValueInfo.cpp

1152

I've been looking at this and despite my efforts, I can't get this approach to generate the same constant ranges as my patch. Here's your implementation with some fixes and modifications to make it work for sgt but I can't see how you can just use one implementation that covers the permutations needed.

static std::optional<ConstantRange>
getRangeViaSLT(CmpInst::Predicate Pred, APInt RHS,
               function_ref<std::optional<ConstantRange>(const APInt &)> Fn) {
  bool Invert = false;
  if (Pred == ICmpInst::ICMP_SGT || Pred == ICmpInst::ICMP_SGE) {
    Pred = ICmpInst::getInversePredicate(Pred);
    Invert = true;
  }
  if (Pred == ICmpInst::ICMP_SLE) {
    Pred = ICmpInst::ICMP_SLT;
    if (RHS.isMaxSignedValue())
      return std::nullopt;
    ++RHS;
  }
  assert(Pred == ICmpInst::ICMP_SLT && "Must be signed predicate");
  if (auto CR = Fn(RHS)) {
    dbgs() << "Init CR: " << *CR << "\n";
    if (Invert) {
      CR = ConstantRange(CR->getUpper(), ~CR->getLower());
    }
    return CR;
  }
  return std::nullopt;
}

// Recognize:
// icmp sgt (ashr X, ShAmtC), C --> icmp sgt X, ((C + 1) << ShAmtC) - 1
// and friends.
// Preconditions: (C != SIGNED_MAX) &&
//                ((C+1) << ShAmtC != SIGNED_MIN) &&
//                (((C+1) << ShAmtC) >> ShAmtC) == (C+1)
const APInt *ShAmtC;
if (CmpInst::isSigned(EdgePred) &&
    match(LHS, m_AShr(m_Specific(Val), m_APInt(ShAmtC))) &&
    match(RHS, m_APInt(C))) {
  auto CR = getRangeViaSLT(
      EdgePred, *C, [&](const APInt &RHS) -> std::optional<ConstantRange> {
        APInt New = RHS << *ShAmtC;
        if ((New.ashr(*ShAmtC)) != RHS)
          return std::nullopt;
        return ConstantRange::getNonEmpty(
            APInt::getSignedMinValue(C->getBitWidth()), New/* + 1*/);
      });
  if (CR) {
    dbgs() << ICI->getFunction()->getName() << ": " << *ICI << "\n";
    dbgs() << "CR is " << *CR << "\n";
    return ValueLatticeElement::getRange(*CR);
  }
}

I may have just misunderstood completely though. But from what I can tell, with the changes needed to make this correct, you'd just end up with code that's harder to follow and just as verbose as before?

[aemerson]

@nikic do you have any more thoughts?

[aemerson]

ping

[aemerson]

ping

[aemerson]

Committed in 9189822f2c2eca55f6e7adcd26e3f15d65797b76