[PATCH] D34207: [IndVarSimplify] Add AShr exact flags using induction variables ranges.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:529
+  // match (X << IVOperand) >> C, marking the AShr as exact using the
+  // information from the IV's range
+  if(BO->getOpcode() == Instruction::Shl) {
----------------
Please clang-format the change.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:532
+    bool Changed = false;
+    for(auto* user : BO->users()) {
+      BinaryOperator* AShr = dyn_cast<BinaryOperator>(user);
----------------
LLVM style is `auto *User` or `auto *U` (I prefer the latter since there is also a `llvm::User`).


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:535
+      if(!AShr || AShr->getOpcode() != Instruction::AShr ||
+          AShr->getOperand(0) != BO || AShr->isExact())
+        continue;
----------------
Use `llvm::PatternMatch` here.  You can then fold in the `dyn_cast<ConstantInt>(AShr->getOperand(1))` check into the same patternmatch expression.

If you do the `dyn_cast<ConstantInt>(AShr->getOperand(1))` check, then the `AShr->getOperand(0) != BO` check should not be necessary.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:542
+      if(ShlOp1 == IVOperand || !BO->getType()->isIntegerTy() ||
+          IVRange.getUpper().uge(BO->getType()->getPrimitiveSizeInBits()))
+        continue;
----------------
Do you really mean `getUpper` or do you want `getUnsignedMax`?

Secondly, `BO` will always be an integer (as opposed to a vector of integers); since the induction variable is an integer.

Thirdly, if I'm not missing something, the `IVRange.getUpper().uge(BO->getType()->getPrimitiveSizeInBits())` check should be unnecessary.  If the IV is larger than the bitwidth then `BO` is poison, and so is `AShr` (and making it exact does not make it any more poison).


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:545
+
+      ConstantInt* AShrOp2 = dyn_cast<ConstantInt>(AShr->getOperand(1));
+      if(!AShrOp2 || IVRange.getLower().ult(AShrOp2->getValue()))
----------------
In cases like this, I think a more indented version is easier to read:

```
if (IVOperand == BO->getOperand(1) && IVMax.ult(BitWidth))
  if (auto *AShrCI = dyn_cast<ConstantInt>(AShr->getOperand(1))
    if (IVMin.uge(AShrOp2->getValue()) {
      AShr->setIsExact(true);
      Changed = true;
    }
```



================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:546
+      ConstantInt* AShrOp2 = dyn_cast<ConstantInt>(AShr->getOperand(1));
+      if(!AShrOp2 || IVRange.getLower().ult(AShrOp2->getValue()))
+        continue;
----------------
Perhaps you need `IVRange.getUnsignedMin()` instead of `IVRange.getLower()`?


================
Comment at: test/Transforms/IndVarSimplify/strengthen-overflow.ll:116
+  %shl = shl i32 1, %k.021
+  %shr = ashr i32 %shl, 1
+; CHECK: %shr = ashr exact i32 %shl, 1
----------------
Add a few more tests here, check that

 - We don't do this transform when illegal (e.g. `shl` by 2, `ashr` by 3).
 - We do the transform when the `shl` amount is strictly greater than the `ashr` amount
 - If I'm right about `shl` ing more than the bitwidth amount above, then please add a test case that checks that situation.


================
Comment at: test/Transforms/IndVarSimplify/strengthen-overflow.ll:123
+  %0 = load i32, i32* %arrayidx, align 4
+  %1 = load i32, i32* %arrayidx1, align 4
+  %sub = sub nsw i32 %1, %0
----------------
I think most of the load/store stuff here can be pruned.


https://reviews.llvm.org/D34207