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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:680
+    // more users).
+    if (Current->getNumDefinedValues() == 0)
+      continue;
----------------
fhahn wrote:
> 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.
> 
> 
My understanding that legality of sinking is checked in isFixedOrderRecurrence. It's checked for every use regardless of number of defs. Thus we should fail to recognize FOR in your case (because it rejects any side-effecting instruction currently). Am I missing something?




================
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'
----------------
fhahn wrote:
> I *think* it would also be good to check actual codegen for the test in `llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains.ll`.
Do you suggest to update CHECKs for all cases inside this file?


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


================
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 ]
----------------
fhahn wrote:
> nit: It would be easier to read  if the induction phi would be named `iv`, and the recurrence phis something like `for.X`.
ok. will fix.


================
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
----------------
fhahn wrote:
> does this need `addrspace(1)`? Also, would be good to pass in the base pointer as arg, to avoid UB in test.
ok. will fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151204