This is an archive of the discontinued LLVM Phabricator instance.

[LoopVectorize] Prevent multiple Phis being generated with in-order reductions
ClosedPublic

Authored by kmclaughlin on Apr 15 2021, 8:37 AM.

Details

Summary

When using the -enable-strict-reductions flag where UF>1 we generate multiple
Phi nodes, though only one of these is used as an input to the vector.reduce.fadd
intrinsics. The unused Phi nodes are removed later by instcombine.

This patch changes widenPHIInstruction/fixReduction to only generate
one Phi, and adds an additional test for unrolling to strict-fadd.ll

Diff Detail

Event Timeline

kmclaughlin created this revision.Apr 15 2021, 8:37 AM
kmclaughlin requested review of this revision.Apr 15 2021, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 8:37 AM
david-arm added inline comments.Apr 15 2021, 8:47 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4299

Hi @kmclaughlin, it feels a bit inconsistent that we're not checking for VF.isVector() here, but we do below? Maybe the VF.isVector() check can be folded into the IsOrdered definition above? Or perhaps we can avoid all the extra VF.isVector() checks completely if we move the check into useOrderedReductions?

4729

To be consistent with the other changes above should we add a && VF.isVector() check here too?

kmclaughlin marked 2 inline comments as done.
  • Moved the VF.isVector() check in fixReduction into the into the IsOrdered definition
  • Added VF.isVector() to the IsOrdered definition in widenPHIInstruction
  • Removed affected CHECK lines from the @fadd_strict_unroll test in scalable-strict-fadd.ll
david-arm accepted this revision.Apr 26 2021, 6:23 AM

LGTM! Thanks for making the changes!

llvm/test/Transforms/LoopVectorize/AArch64/strict-fadd.ll
84

nit: Perhaps this should just be {{.*}} instead of bc.merge.rdx?

This revision is now accepted and ready to land.Apr 26 2021, 6:23 AM
This revision was landed with ongoing or failed builds.Apr 28 2021, 3:29 AM
This revision was automatically updated to reflect the committed changes.
kmclaughlin marked an inline comment as done.