[PATCH] D137840: [ConstraintElimination] Dummy revision for review

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:78
+  StackEntry(unsigned NumIn, unsigned NumOut, bool IsSigned,
+             SmallVector<Value *, 2> ValuesToRelease)
+      : NumIn(NumIn), NumOut(NumOut), IsSigned(IsSigned),
----------------
Is the by-value pass here intentional? If so, should std::move be used in the initializer list?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:187
+  /// True if the variable is known positive in the current constraint.
+  bool IsKnownPositive;
+
----------------
This is probably more accurately named IsKnownNonNegative.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:231
+  const APInt &Val = CI->getValue();
+  return Val.sgt(MinSignedConstraintValue) && Val.slt(MaxConstraintValue);
+}
----------------
I'm wondering whether these should be sge and sle rather than sgt/slt.

Edit: Hm, I guess MinSigned is excluded to avoid issues with various `-1` multiplies?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:243
+
+  if (!GEP.isInBounds())
+    return &GEP;
----------------
Hm, do we need an !IsSigned check here as well, as otherwise the base pointer + offset addition may overflow in a signed sense?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:250
+  if (InnerGEP && GEP.getNumOperands() == 2 &&
+      isa<ConstantInt>(GEP.getOperand(1))) {
+    APInt Offset = cast<ConstantInt>(GEP.getOperand(1))->getValue();
----------------
Rather than checking for a specific GEP structure here, it might be more elegant to use accumulateConstantOffset() to handle any constant offset GEP?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:268
+          ConstantInt::get(InnerGEP->getOperand(1)->getType(),
+                           -1 * Offset.getSExtValue()));
+    }
----------------
This precondition does not look correct. What if the inner GEP has more than one offset? What if it has a non-i8 element type?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:307
+      Preconditions.emplace_back(CmpInst::ICMP_SGE, Index,
+                                 ConstantInt::get(Index->getType(), 0));
+  }
----------------
I think this condition can be incorrect if the index is truncating. For simplicity, maybe just reject indices of incorrect width?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:315
+// pair is the constant-factor and X must be nullptr. If the expression cannot
+// be decomposed, returns an empty vector.
+static Decomposition decompose(Value *V,
----------------
Ooops, this comment is outdated after recent changes.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:399
+  if (match(V, m_NUWSub(m_Value(Op0), m_Value(Op1))))
+    return {0, {{1, Op0}, {-1, Op1}}};
+
----------------
Are there any problems if Op1 is SIGNED_MIN here?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:437
+
+  // Only ULE and ULT predicates are supported at the moment.
+  if (Pred != CmpInst::ICMP_ULE && Pred != CmpInst::ICMP_ULT &&
----------------
Outdated comment.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:507
+  // Remove any (Coefficient, Variable) entry where the Coefficient is 0 for new
+  // variables.
+  while (!NewVariables.empty()) {
----------------
A bit odd that we only do this if the zero coefficients happen to be at the end.

Edit: Ah, I guess we primarily care about determining whether there are no new variables at all here?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:634
+  /// loop header).
+  bool canAddSuccessor(BasicBlock &BB, BasicBlock *Succ) const {
+    if (BB.getSingleSuccessor()) {
----------------
I wonder whether this function is equivalent to `DT.dominates(BasicBlockEdge(&BB, Succ), Succ)`?


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:676
+  // basic block. And to skip another traversal of each basic block when
+  // simplifying.
+  for (Instruction &I : BB) {
----------------
Yeah, that would make more sense to me ...


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:689
+        for (BasicBlock *Succ : successors(&BB)) {
+          if (!canAddSuccessor(BB, Succ))
+            continue;
----------------
In this case, wouldn't a properlyDominates(&BB, Succ) check be sufficient? If we have something like assume; if() {A} B, then the assume holds in B.


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

https://reviews.llvm.org/D137840