[PATCH] D93725: [LV] Relax assumption that LCSSA implies single entry

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:627
 
   /// The Loop exit block may have single value PHI nodes with some
   /// incoming value. While vectorizing we only handled real values
----------------
This also needs updating now, right? The exit may have phis with multiple incoming values and it needs to fix PHIs without incoming values from the  'middle' block.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4137
+  // had multiple exiting edges; in that case, the path through middle will
+  // dynamically dead and the value picked for the phi doesn't matter.
+  for (PHINode &LCSSAPhi : LoopExitBlock->phis())
----------------
This is because we run the last iteration in the scalar loop, right? I think it would be good to extend the comment. Also, the same comment also applies to the similar code in `fixReduction` and `fixLCSSAPHIs` as well?

And could you add a test case where we have LCSSA phis where some incoming values are different to the recurrence, as in 


```
define i16 @test(i16* %p, i32 %n) {
entry:
  br label %for.cond

for.cond:
  %i = phi i32 [ 0, %entry ], [ %inc, %for.body ]
  %rec = phi i16 [0, %entry], [ %rec.next, %for.body ]
  %iprom = sext i32 %i to i64
  %b = getelementptr inbounds i16, i16* %p, i64 %iprom
  %rec.next = load i16, i16* %b
  %cmp = icmp slt i32 %i, %n
  br i1 %cmp, label %for.body, label %if.end

for.body:
  store i16 %rec , i16* %b, align 4
  %inc = add nsw i32 %i, 1
  %cmp2 = icmp slt i32 %i, 2096
  br i1 %cmp2, label %for.cond, label %if.end

if.end:
  %rec.lcssa = phi i16 [ %rec, %for.cond ], [ 10, %for.body ]
  ret i16 %rec.lcssa
}
```


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4139
+  for (PHINode &LCSSAPhi : LoopExitBlock->phis())
+    for (Value *V : LCSSAPhi.incoming_values())
+      if (V == Phi) {
----------------
nit: personally I find using `any_of` slightly more explicit, like

```
    if (any_of(LCSSAPhi.incoming_values(), [Phi](Value *V) { return V == Phi; }))
      LCSSAPhi.addIncoming(ExtractForPhiUsedOutsideLoop, LoopMiddleBlock);
```

but that's down to personal preference


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4404
+    if (LCSSAPhi.getBasicBlockIndex(LoopMiddleBlock) != -1)
+      // Some phis were already hand updated by the induction and recurrence
+      // code above, leave them alone.
----------------
Should that be `reduction and recurrence code above`?


================
Comment at: llvm/test/Transforms/LoopVectorize/loop-form.ll:349
 ; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = icmp sgt i32 [[N:%.*]], 0
+; CHECK-NEXT:    [[SMAX:%.*]] = select i1 [[TMP0]], i32 [[N]], i32 0
----------------
can you also add a test with multiple unique exits & first-order recurrences/reductions?

like

```
define i16 @multiple_unique_exit_fof(i16* %p, i32 %n) {
entry:
  br label %for.cond

for.cond:
  %i = phi i32 [ 0, %entry ], [ %inc, %for.body ]
  %rec = phi i16 [0, %entry], [ %rec.next, %for.body ]
  %iprom = sext i32 %i to i64
  %b = getelementptr inbounds i16, i16* %p, i64 %iprom
  %rec.next = load i16, i16* %b
  %cmp = icmp slt i32 %i, %n
  br i1 %cmp, label %for.body, label %if.end

for.body:
  store i16 %rec , i16* %b, align 4
  %inc = add nsw i32 %i, 1
  %cmp2 = icmp slt i32 %i, 2096
  br i1 %cmp2, label %for.cond, label %if.end

if.end:
  ret i16 %rec
}
```

Not sure if `loop-form.ll` would be the right place, perhaps adding them to `llvm/test/Transforms/LoopVectorize/first-order-recurrence.ll` and `llvm/test/Transforms/LoopVectorize/reduction.ll` respectively would be better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93725