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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:680
+    // more users).
+    if (Current->getNumDefinedValues() == 0)
+      continue;
----------------
ebrevnov wrote:
> 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?
> 
> 
This is not longer completely true, whether the users of recurrences can be re-ordered or not is checked based on VPlan, which is more powerful. Note that the test case hasn't been vectorized before re-ordering based on VPlan (see https://llvm.godbolt.org/z/8d9PM353T). Until we check for aliasing based on VPlan (patches mentioned in earlier comments), we should bail out when trying to sink recipes that may have side effects.


================
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'
----------------
ebrevnov wrote:
> 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?
No, adding a separate test here `llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains.ll`, but it is probably not needed, checking the VPlan only is simpler. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151204