Page MenuHomePhabricator

[LoopVectorize] Only check for ordered reductions if the op chain was found

Authored by kmclaughlin on Jan 14 2022, 6:48 AM.



As we cannot vectorise strict reductions if the reduction is not also
classified as in-loop, we should not call checkOrderedReductions
if getReductionOpChain was not able to find the chain.

This removes the additional check on the number of uses of Exit in
checkOrderedReductions introduced in D114002, added to fix an issue
where IsOrdered was true but the reduction op chain was not found.

Diff Detail

Event Timeline

kmclaughlin created this revision.Jan 14 2022, 6:48 AM
kmclaughlin requested review of this revision.Jan 14 2022, 6:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2022, 6:48 AM

The patch looks good to me @kmclaughlin! I just had one suggestion about maybe leaving the function where it was before.


Maybe it's better to leave this function where it was originally so that it's easier to see what's changed? It seems to be just the declaration that's different I think?

kmclaughlin marked an inline comment as done.
  • Moved getReductionOpChain back to the original location in IVDescriptors.cpp
david-arm added inline comments.Jan 17 2022, 8:40 AM

nit: Thanks for addressing my previous comment - it looks a lot neater now and easier to review. :) Given that RecurrenceDescriptor now has both getReductionOpChain and getReductionChain functions, is it worth renaming getReductionOpChain to something like findReductionOpChain?


Sorry I missed this before - perhaps this change shouldn't be part of this patch because it looks like it may be adding new cases that aren't being tested for correctness?

kmclaughlin marked 2 inline comments as done.
kmclaughlin edited the summary of this revision. (Show Details)
  • Renamed getReductionOpChain -> findReductionOpChain
  • Added !LoopExitInstr->hasNUses(2) back into findReductionOpChain
david-arm accepted this revision.Jan 20 2022, 1:59 AM

LGTM! Thanks for addressing my comments @kmclaughlin. :)


nit: Could you fix the formatting before merging? Thanks!

This revision is now accepted and ready to land.Jan 20 2022, 1:59 AM
fhahn added a subscriber: fhahn.Jan 20 2022, 2:09 AM

It sounds like this fixes an issue, is it possible to have a test?

fhahn added a comment.Jan 20 2022, 2:10 AM

Or is it NFC?

Hi @fhahn, there was a test added in D114002 which covers the changes here. This is just a follow up from some discussion on that patch.