Page MenuHomePhabricator

[LoopVectorize] Fix strict reductions where VF = 1
ClosedPublic

Authored by kmclaughlin on Jun 18 2021, 6:38 AM.

Details

Summary

Currently we will allow loops with a fixed width VF of 1 to vectorize
if the -enable-strict-reductions flag is set. However, the loop vectorizer
will not use ordered reductions if VF.isScalar() and the resulting
vectorized loop will be out of order.

This patch removes VF.isVector() when checking if ordered reductions
should be used. Also, instead of converting the FAdds to reductions if the
VF = 1, operands of the FAdds are changed such that the order is preserved.

Diff Detail

Event Timeline

kmclaughlin created this revision.Jun 18 2021, 6:38 AM
kmclaughlin requested review of this revision.Jun 18 2021, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2021, 6:38 AM
sdesmalen added inline comments.Jun 18 2021, 8:54 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9281

Is this a valid change for in-loop reductions that are not in-order? Or does this now break things for VF=1, UF>1 for in-loop reductions?

9356

nit: MinVF ?

9382–9383

nit: assert ((VF.isScalar() || isa<VPWidenRecipe>(WidenRecipe)) && ...)

9404

Should these asserts be guarded as well?

kmclaughlin marked 2 inline comments as done.
  • Changed adjustRecipesForInLoopReductions so that for a VF of 1 we only use VPReductionRecipe if the reduction is also ordered.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9281

Hi @sdesmalen, thanks for pointing this out, this will break things for in-loop reductions where VF=1 & UF>1. I've changed adjustRecipesForInLoopReductions so that we only add a VPReductionRecipe where VF.isScalar if useOrderedReductions is also true.

9404

These asserts are only for only for min/max recurrences, so we should never reach these for ordered reductions.

bmahjour removed a subscriber: bmahjour.Jun 21 2021, 9:22 AM
david-arm accepted this revision.Jun 24 2021, 6:59 AM

LGTM!

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

I think this should be fine. If we ever do add support for in-order min/max reductions then this assert (and the other one above - isa<VPWidenRecipe>(WidenRecipe)) will fire and we can fix it up then.

This revision is now accepted and ready to land.Jun 24 2021, 6:59 AM
This revision was landed with ongoing or failed builds.Jun 28 2021, 3:28 AM
This revision was automatically updated to reflect the committed changes.