Page MenuHomePhabricator

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

Authored by kmclaughlin on Thu, Apr 15, 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.Thu, Apr 15, 8:37 AM
kmclaughlin requested review of this revision.Thu, Apr 15, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Apr 15, 8:37 AM
david-arm added inline comments.Thu, Apr 15, 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.Mon, Apr 26, 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.Mon, Apr 26, 6:23 AM
This revision was landed with ongoing or failed builds.Wed, Apr 28, 3:29 AM
This revision was automatically updated to reflect the committed changes.
kmclaughlin marked an inline comment as done.