Page MenuHomePhabricator

[LoopVectorize] Fix assertion failure in fixReduction when tail-folding
ClosedPublic

Authored by david-arm on May 24 2022, 6:17 AM.

Details

Summary

When compiling the attached new test in scalable-reductions-tf.ll we
were hitting this assertion in fixReduction:

Assertion `isa<PHINode>(U) && "Reduction exit must feed Phi's or select"

The loop contains a reduction and an intermediate store of the reduction
value. When vectorising with tail-folding the contains of 'U' in the
assertion above happened to be a scatter_store. It turns out that we
were still creating a widen recipe for the invariant store, despite
knowing that we can actually sink it. The simplest fix is to change
buildVPlanWithVPRecipes so that we look for invariant stores before
attempting to widen it.

Diff Detail

Event Timeline

david-arm created this revision.May 24 2022, 6:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 6:17 AM
david-arm requested review of this revision.May 24 2022, 6:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 6:17 AM
sdesmalen accepted this revision.May 24 2022, 7:09 AM
This revision is now accepted and ready to land.May 24 2022, 7:09 AM
fhahn accepted this revision.May 24 2022, 7:54 AM
fhahn added a subscriber: fhahn.

LGTM, thanks! It would probably be more descriptive to say how the issue is fixed in the commit message, e.g. Skip stores of reduction values before trying to widen or something like that.

llvm/test/Transforms/LoopVectorize/AArch64/scalable-reductions-tf.ll
3

no need to add +bf16?

4

is there any particular reason you are checking for remarks?

30

the control flow in the loop shouldn't be needed.

50

%if.then4 comment here seems out of date, if.then4 doesn't exist? Same for for.body.lr.ph above.

david-arm updated this revision to Diff 431688.May 24 2022, 8:52 AM
  • Address review comments on test.
david-arm marked 4 inline comments as done.May 24 2022, 8:53 AM
david-arm added inline comments.
llvm/test/Transforms/LoopVectorize/AArch64/scalable-reductions-tf.ll
4

No reason I guess - I just used scalable-reductions.ll as a basis for this test.

This revision was landed with ongoing or failed builds.May 25 2022, 3:47 AM
This revision was automatically updated to reflect the committed changes.
david-arm marked an inline comment as done.