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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for taking a look! I pushed a couple of commits that addressed some points and also put up D139482 <https://reviews.llvm.org/D139482>.



================
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),
----------------
nikic wrote:
> Is the by-value pass here intentional? If so, should std::move be used in the initializer list?
Added `std::move`, but hopefully compilers should be able to generate good code after inlining even without it :)


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


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:231
+  const APInt &Val = CI->getValue();
+  return Val.sgt(MinSignedConstraintValue) && Val.slt(MaxConstraintValue);
+}
----------------
nikic wrote:
> 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?
Yeah, `Val.sgt(MinSignedConstraintValue) ` ensures multiplying constants with `-1` won't overflow. The upper bound could be `sle`.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:243
+
+  if (!GEP.isInBounds())
+    return &GEP;
----------------
nikic wrote:
> Hm, do we need an !IsSigned check here as well, as otherwise the base pointer + offset addition may overflow in a signed sense?
At the moment, this function isn't called for Signed, but I'll add an assert. I also added variants of the existing GEP tests but with signed predicates.


================
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();
----------------
nikic wrote:
> Rather than checking for a specific GEP structure here, it might be more elegant to use accumulateConstantOffset() to handle any constant offset GEP?
I updated the code to use `collectOffset` here and instead of the loop over GEP indices below in 

ee605b0accce
6a834d2f2b96


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:268
+          ConstantInt::get(InnerGEP->getOperand(1)->getType(),
+                           -1 * Offset.getSExtValue()));
+    }
----------------
nikic wrote:
> 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?
Should be fixed in ee605b0accce by normalizing the offset. I tried various cases to see if I could construct a test case that fails verification without the condition, as `inbounds` GEPs shouldn't wrap around `0` when decrementing, hence we should be able to model this without the precondition I think. I'll plan to submit a patch for that separately.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:307
+      Preconditions.emplace_back(CmpInst::ICMP_SGE, Index,
+                                 ConstantInt::get(Index->getType(), 0));
+  }
----------------
nikic wrote:
> I think this condition can be incorrect if the index is truncating. For simplicity, maybe just reject indices of incorrect width?
To my slight surprise, the implicit truncate preserves the signed value according to langref, so I think this should be fine?

https://llvm.org/docs/LangRef.html#getelementptr-instruction

> If the type of an index is larger than the pointer index type, the truncation to the pointer index type preserves the signed value.



================
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,
----------------
nikic wrote:
> Ooops, this comment is outdated after recent changes.
Updated in 6b940588a0fc, thanks!


================
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}}};
+
----------------
nikic wrote:
> Are there any problems if Op1 is SIGNED_MIN here?
No, I think the only issue would be if we multiply `SINGED_MIN` constant with `-1`, but that's avoided above. If Op1 is `SINGED_MIN` here, it will be treated as variable.


================
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 &&
----------------
nikic wrote:
> Outdated comment.
Will remove, thanks!


================
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()) {
----------------
nikic wrote:
> 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?
Yep exactly, e.g. this is the case when one side adds and the other side subtracts the same value.


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


================
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) {
----------------
nikic wrote:
> Yeah, that would make more sense to me ...
Updated in current `main`.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:689
+        for (BasicBlock *Succ : successors(&BB)) {
+          if (!canAddSuccessor(BB, Succ))
+            continue;
----------------
nikic wrote:
> 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.
This is not longer needed in the latest version after addressing the TODO above.


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

https://reviews.llvm.org/D137840