[PATCH] D154069: [IRCE][NFC] Parse range checks in the form of "LHS - RHS vs Limit"

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:376
+  // TODO: Can we remove this restriction?
+  if (!SE.willNotOverflow(Instruction::BinaryOps::Sub, ICmpInst::isSigned(Pred),
+                          IV, Offset, cast<Instruction>(VariantLHS))) {
----------------
More context here in comment. Why with this restriction failed we cannot process.


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:395
+
+  // We are going to reassociate expression but the computations can overflow.
+  // To avoid it, let's use extended type for all operands and then add runtime
----------------
What exactly overflow you can expect?
Does it makes sense to check, may be we can prove that overflow is not possible and we can avoid scaling?


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:424
+    // We multiply by Scale because it's 1 or -1
+    assert(Scale->isOne() || Scale->isAllOnesValue());
+    End = SE.getMulExpr(SE.getMinusSCEV(Limit, Offset), Scale);
----------------
Add some message


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1688
+    // TODO: Support runtime overflow check for End
+    return std::nullopt;
+  }
----------------
Can we add some LLVM_DEBUG output here and add a test which tests whether it is printed and check the values of getBegin(), getEnd() and IndVar?

It would be nice to have tests for this functionality?


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

https://reviews.llvm.org/D154069