[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:
> > 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.
> > This is not longer completely true, ...
> I understand the intent to do legality checking on VPlan, but at the moment it doesn't seem to be fully the case. In particular, there is a check that sink candidate doesn't have side effects in RecurrenceDescriptor::isFixedOrderRecurrence:
> 
> ```
>     if (SinkCandidate->getParent() != PhiBB ||
>         SinkCandidate->mayHaveSideEffects() ||
>         SinkCandidate->mayReadFromMemory() || SinkCandidate->isTerminator())
>       return false;
> 
> ```
>  That means we can't meet side effecting user of FOR in sinkRecurrenceUsersAfterPrevious. So I don't understand why we need another check here.
> 
> Maybe,  what you mean is to move the check from RecurrenceDescriptor::isFixedOrderRecurrence to sinkRecurrenceUsersAfterPrevious? Could you clarify.
> 
> PS: It looks like comment for 'adjustFixedOrderRecurrences' is a bit outdated since it still assumes that legality is checked before.
> 
> >   /// The current implementation assumes all users can be sunk after the previous
> >   /// value, which is enforced by earlier legality checks.
> 
> 
> That means we can't meet side effecting user of FOR in sinkRecurrenceUsersAfterPrevious. So I don't understand why we need another check here.

This is true in most cases, but your test case uncovered a scenario where this is not the case. Before `sinkRecurrenceUsersAfterPrevious` was introduced to handle the re-ordering, the code was unable to reorder the code in the  test case due to limitations in the old implementation. 

Now we can re-order, but this means we need to check for side effects to catch cases where the original order of instructions doesn't require sinking an instruction with side effects, but does so after sinking operands for a different FOR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151204