[PATCH] D152730: [ConstraintElim] Add A < B if A is an increasing phi for A != B.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1166
+  if (Pred != CmpInst::ICMP_NE || !PN || PN->getNumIncomingValues() != 2)
+    return;
+
----------------
nikic wrote:
> TODO: Handle phi on RHS? I don't think any canonicalization guarantees that it is on the LHS.
Added a TODO, should be fairly straigth-forward to extend.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1187
+    auto *N = DT.getNode(Succ);
+    if (NumIn == N->getDFSNumIn() && NumOut == N->getDFSNumOut())
+      InLoopSucc = Succ;
----------------
nikic wrote:
> Wonder whether it would be better to store the DTN in FactOrCheck rather than NumIn/NumOut.
The main reason for storing the numbers directly is to avoid unnecessary indirection when sorting the worklist, which probably outweighs the benefit here of having easy access to DTN, but I might be wrong.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1194
+      LI.getLoopFor(LoopExitSucc) == L)
+    return;
+
----------------
nikic wrote:
> Generally, this whole "look at branch of single user of comparison" approach looks pretty hacky. This seems like something that should be done when initially queueing the fact, as we have direct knowledge of the branch structure at that point.
It would be slightly cleaner to do it a queuing time. There are 2 current limitations that prevent us from doing so:

1. We would need a way to add a precondition for facts that needs to be checked just before adding a fact (`StartValue <= B`)
2. A way track synthetic facts (e.g. `PN < B` which doesn't exist as ICmpInst)




================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1222
+      DL.getIndexTypeSizeInBits(StepInst->getPointerOperand()->getType());
+  MapVector<Value *, APInt> StepVariableOffsets;
+  APInt StepOffset(BitWidth, 0);
----------------
nikic wrote:
> Unused variable
Removed, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1235
+    unsigned UpperBitWidth =
+        DL.getIndexTypeSizeInBits(UpperGEP->getPointerOperand()->getType());
+    MapVector<Value *, APInt> UpperVariableOffsets;
----------------
nikic wrote:
> Must be same as BitWidth?
Yep, simplified, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1250
+
+  // We know that PN != B, so if Start <= B we know that PN < B and PN >=
+  // StartValue.
----------------
nikic wrote:
> Start -> StartValue
Updated, thanks!


================
Comment at: llvm/test/Transforms/PhaseOrdering/iterator-with-runtime-check.ll:43
 ; CHECK-NEXT:    [[CMP_I_NOT:%.*]] = icmp eq ptr [[INCDEC_PTR_I]], [[ADD_PTR_I]]
 ; CHECK-NEXT:    br i1 [[CMP_I_NOT]], label [[COMMON_RET]], label [[FOR_BODY]]
 ;
----------------
nikic wrote:
> I'm a bit confused on how this one ends up working. Here we have the condition on the latch, so it doesn't dominate for.body. How can we still eliminate a condition there?
ConstraintElimination runs before loop-rotate, it processes and simplifies the following IR:

```
define void @test_fill_with_foreach([2 x i64] %elems.coerce) local_unnamed_addr {                        <<<
entry:
  %elems.coerce.fca.0.extract = extractvalue [2 x i64] %elems.coerce, 0
  %0 = inttoptr i64 %elems.coerce.fca.0.extract to ptr
  %elems.coerce.fca.1.extract = extractvalue [2 x i64] %elems.coerce, 1
  %add.ptr.i = getelementptr inbounds i32, ptr %0, i64 %elems.coerce.fca.1.extract
  %cmp.not.i.i.i.i = icmp slt i64 %elems.coerce.fca.1.extract, 0
  br i1 %cmp.not.i.i.i.i, label %error, label %for.cond

common.ret:                                       ; preds = %for.cond, %error
  ret void

error:                                            ; preds = %for.body, %entry
  call void @error()
  br label %common.ret

for.cond:                                         ; preds = %entry, %for.latch
  %__begin1.sroa.0.0 = phi ptr [ %incdec.ptr.i, %for.latch ], [ %0, %entry ]
  %cmp.i.not = icmp eq ptr %__begin1.sroa.0.0, %add.ptr.i
  br i1 %cmp.i.not, label %common.ret, label %for.body

for.body:                                         ; preds = %for.cond
  %cmp.not.i.i = icmp uge ptr %__begin1.sroa.0.0, %0
  %cmp2.i.i = icmp ult ptr %__begin1.sroa.0.0, %add.ptr.i
  %sel = select i1 %cmp.not.i.i, i1 %cmp2.i.i, i1 false
  br i1 %sel, label %for.latch, label %error

for.latch:                                        ; preds = %for.body
  call void @use(ptr noundef nonnull align 4 dereferenceable(4) %__begin1.sroa.0.0)
  %incdec.ptr.i = getelementptr inbounds i32, ptr %__begin1.sroa.0.0, i64 1
  br label %for.cond
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152730