This is an archive of the discontinued LLVM Phabricator instance.

[LoopVectorize] Check the number of uses of an FAdd before classifying as ordered
ClosedPublic

Authored by kmclaughlin on Nov 16 2021, 7:20 AM.

Details

Summary

checkOrderedReductions looks for Phi nodes which can be classified as in-order,
meaning they can be vectorised without unsafe math. In order to vectorise the
reduction it should also be classified as in-loop by getReductionOpChain, which
checks that the reduction has two uses.

In this patch, a similar check is added to checkOrderedReductions so that we
now return false if there are more than two uses of the FAdd instruction.
This fixes PR52515.

Diff Detail

Event Timeline

kmclaughlin created this revision.Nov 16 2021, 7:20 AM
kmclaughlin requested review of this revision.Nov 16 2021, 7:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2021, 7:20 AM
fhahn accepted this revision.Nov 16 2021, 7:37 AM

LGTM with the comment addressed

llvm/lib/Analysis/IVDescriptors.cpp
203

please use hasNUsesOrMore instead of getNumUses. hasNUsesOrMore doesn't have to traverse the full use list.

This revision is now accepted and ready to land.Nov 16 2021, 7:37 AM

Thanks for putting together the fix.

I don't remember why the LoopExitValue needs a single user for InLoopReductions. It should have one Phi use, and only other uses outside loop, but I don't think it really needs to be a single user unless I'm missing something.

The underlying issue seems to be that AddReductionVar returns a reduction, whereas getReductionOpChain cannot find the chain. Are there more situations that could happen? Would it be better to somehow detect that getReductionOpChain will fail to find the chain?

The underlying issue seems to be that AddReductionVar returns a reduction, whereas getReductionOpChain cannot find the chain. Are there more situations that could happen? Would it be better to somehow detect that getReductionOpChain will fail to find the chain?

Thanks for taking a look at this, @dmgreen! I do think it would be worth checking if getReductionOpChain will fail to find the chain in AddReductionVar, so that we don't classify a reduction as ordered if it won't be found by collectInLoopReductions. I tried adding something like this to AddReductionVar, though at the point checkOrderedReduction is called we haven't created the RecurrenceDescriptor yet. I think with some refactoring it would be possible to do this, and I'm happy to follow up on this separately? For now I think this patch ensures that we are at least as strict as getReductionOpChain in terms of checking the number of uses of the FAdd.

david-arm accepted this revision.Nov 18 2021, 7:11 AM

LGTM! I think this fix looks right for now, especially since it will unblock the users who reported the original issue in bugzilla. It does sound like cleaning up this area with a bit of refactoring and removing duplication would make sense though!

Yeah that sounds fine. I took another look and checkOrderedReduction is already checking for a small subset of what getReductionOpChain is testing for. It would be nice that if we fixed getReductionOpChain to not require a single user it would just work without extra changes, but it's simple enough to fix both places.

This revision was landed with ongoing or failed builds.Nov 18 2021, 8:43 AM
This revision was automatically updated to reflect the committed changes.
kmclaughlin marked an inline comment as done.