Page MenuHomePhabricator

[LV] Support sinking recipe in replicate region after another region.
ClosedPublic

Authored by fhahn on Jun 2 2021, 5:10 AM.

Details

Summary

This patch handles sinking a replicate region after another replicate
region. In that case, we can connect the sink region after the target
region. This properly handles the case for which an assertion has been
added in 337d7652823f.

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34842.

Diff Detail

Event Timeline

fhahn created this revision.Jun 2 2021, 5:10 AM
fhahn requested review of this revision.Jun 2 2021, 5:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2021, 5:10 AM
fhahn updated this revision to Diff 350838.Jun 9 2021, 3:05 AM

Rebase & ping :)

Ayal added a comment.Jun 10 2021, 6:47 AM

Minor comments and suggestions. This should compete the handling of sinking a recipe after another, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9206

Set SinkRegion and TargetRegion once at the outset?

9207

Have this simplest and most common (!SinkRegion && !TargetRegion) case early-exit first?
Followed by the next simple (!SinkRegion && TargetRegion) case; both move the Sink recipe via Sink->move*();
Last deal with SinkRegion by first disconnecting it and then placing it between an AfterBlock and its single successor, where AfterBlock is produced by splitting the Target block if !TargetRegion.

9208

These asserts are better placed inside the lambda?

9211

// If the recipe >> // The recipe

"recpliate"

9217

same asserts...

9219–9220

same assert...

9219–9220

This part of disconnecting SinkRegion could be done once for both TargetRegion and !TargetRegion cases.

9230

// If the target >> // The target

llvm/test/Transforms/LoopVectorize/first-order-recurrence-sink-replicate-region.ll
388

%rem and %rem.div both need to sink after Previous = %recur.next (which is invariant...), and do so by first having %rem move after Previous and then having %rem.div move after %rem, where this last move is the one that moves a replicated region after another, right?

fhahn updated this revision to Diff 351654.Jun 12 2021, 6:12 AM
fhahn marked an inline comment as done.

Thanks for all the comments Ayal! I updated the code and removed some redundancies as suggested.

fhahn marked 4 inline comments as done.Jun 12 2021, 6:13 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9207

I restructured the code to handle the simplest case first, then SinkRegion and then TargetRegion.

9208

I moved the SESE one. The other one would be a bit more work, because currently the lambda does not have access to Target's/Sink's parents. A general helper to check for replicate regions in the future may be helpful.

Ayal added inline comments.Jun 16 2021, 1:26 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9208

Lambda can check if R->getParent() has size 1, right? This should be asserted for both sink and target.

9240

Handle the "SinkRegion && !TargetRegion" case here, splitting TargetBlock etc., instead of last below?

9242

Handle this relatively simple "!SinkRegion && TargetRegion" case 2nd, after the simplest !SinkRegion && !TargetRegion? (I.e., can check first if (!SinkRegion), etc.)

fhahn updated this revision to Diff 352729.Jun 17 2021, 7:52 AM
fhahn marked an inline comment as done.

Restructure checks as suggested and moved the assertion to lambda, thanks!

fhahn marked 4 inline comments as done.Jun 17 2021, 7:55 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9208

Yes, I moved the assertion,

9240

Done, thanks!

9242

I restructured the code, it should be a bit more compact now.

fhahn marked 2 inline comments as done.Jun 23 2021, 7:26 AM

ping :)

Ayal accepted this revision.Jun 23 2021, 11:03 AM

Looks good to me, thanks for accommodating the changes!
Adding a minor error message suggestion.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9199

"parent of R must be a replicator with a single recipe" >>
"A recipe in an original replicator region must be the only recipe in its block"?
("original" - because this does not hold for merged replicator regions).

This revision is now accepted and ready to land.Jun 23 2021, 11:03 AM
fhahn added a comment.Jun 24 2021, 5:57 AM

Thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9199

Sounds good, I'll update the message when committing!