[PATCH] D139790: [VPlan] Consider all recipes in replicate blocks as sink candidates.

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 10 02:04:45 PST 2023


fhahn reclaimed this revision.
fhahn marked 3 inline comments as done.
fhahn added a comment.
Herald added a subscriber: StephenFan.

Re-open the revision as after discussion this approach seems preferable to aa2414729ebb <https://reviews.llvm.org/rGaa2414729ebbcb2d8f162e9002a3a6aa768b1f9d>



================
Comment at: llvm/test/Transforms/LoopVectorize/vplan-sink-scalars-and-merge.ll:38
+; CHECK-NEXT:     vp<[[STEPS:%.+]]> = SCALAR-STEPS vp<[[CAN_IV]]>, ir<1>
 ; CHECK-NEXT:     REPLICATE ir<%gep.b> = getelementptr ir<@b>, ir<0>, vp<[[STEPS]]>
 ; CHECK-NEXT:     REPLICATE ir<%lv.b> = load ir<%gep.b>
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > fhahn wrote:
> > > > Ayal wrote:
> > > > > The iterative scan up use-def chains stops at GEPs?
> > > > At the moment only replicate & scalar-steps recipe can be sunk.
> > > Right, but curious why the use-def scan failed to sink vp<[[STEPS]]> used by sunk REPLICATEs of GEPS while scanning all recipes in replicate block succeeded. Is this due to the now iterative nature of applying sinkScalarOperands()?
> > Ah right! I had another look and the issue was that operands of instructions that do not need sinking aren't added to the worklist. Should be fixed by aa2414729ebb
> Hmm, that may have been intentional - to prevent revisiting same candidates multiple times during a scan up use-def chains. But that assumed operands that do not need sinking were sunk in the "current" scan - when activated only once, starting with a single seed per replicate region. Whereas now the first scan starts with a single seed but repeated iterative scans should indeed consider all / more-than-one?
As discussed, aa2414729ebb has been reverted and the current patch seems preferable; 

> Whereas now the first scan starts with a single seed but repeated iterative scans should indeed consider all / more-than-one?

Yes, now `sinkScalarOperands` may be called for triangles with multiple instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139790



More information about the llvm-commits mailing list