[PATCH] D151204: [VPlan] Allow sinking of instructions with no defs

[Florian Hahn via Phabricator via llvm-commits]

fhahn added reviewers: Ayal, gilr.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:680
+    // more users).
+    if (Current->getNumDefinedValues() == 0)
+      continue;
----------------
I *think* this would not be safe in all cases; e.g. if we have a `store` here than we would need to sink a store, which may not be legal if there are other loads in the loop and we would sink past them (would be good to add a test case)

I *think* to start with, it would be good to prevent sinking instructions with side-effects in `TryPushSinkCandidate`.

There's a patch series that moves memory legality checking to VPlan (D142895 and following) which support s sinking some memory operations.




================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains-vplan.ll:112
+
+define i32 @test_chained_first_order_recurrences_4() {
+; CHECK-LABEL: 'test_chained_first_order_recurrences_4'
----------------
I *think* it would also be good to check actual codegen for the test in `llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains.ll`.


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains-vplan.ll:151
+
+bb347:                                            ; preds = %bb347, %bb
+  %phi348 = phi i64 [ %add351, %bb347 ], [ 0, %bb ]
----------------
nit: using `loop` as block name would be more descriptive.


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains-vplan.ll:152
+bb347:                                            ; preds = %bb347, %bb
+  %phi348 = phi i64 [ %add351, %bb347 ], [ 0, %bb ]
+  %phi349 = phi i64 [ %mul368, %bb347 ], [ 0, %bb ]
----------------
nit: It would be easier to read  if the induction phi would be named `iv`, and the recurrence phis something like `for.X`.


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains-vplan.ll:156
+  %add351 = add i64 %phi348, 1
+  %getelementptr354 = getelementptr i64, ptr addrspace(1) null, i64 %phi348
+  %trunc355 = trunc i64 %phi349 to i32
----------------
does this need `addrspace(1)`? Also, would be good to pass in the base pointer as arg, to avoid UB in test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151204