[PATCH] D151911: [LVI] Handle icmp of ashr.

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/Analysis/LazyValueInfo.cpp:1152
+  // Recognize:
+  // icmp sgt (ashr X, ShAmtC), C --> icmp sgt X, ((C + 1) << ShAmtC) - 1
+  // and friends.
----------------
nikic wrote:
> 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.
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151911