This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][ARM] Always expand ordered vector reductions (PR44600)
ClosedPublic

Authored by nikic on Jan 21 2020, 1:00 PM.

Details

Summary

fadd/fmul reductions without reassoc are lowered to VECREDUCE_STRICT_FADD/FMUL nodes, which currently have zero legalization support. Until that is in place, force them to be expanded so we don't get legalization assertions.

This partially fixes https://bugs.llvm.org/show_bug.cgi?id=44600. The other part of the issue is fmax/fmin NaN handling on AArch64. This seems to be underspecified right now, as langref doesn't tell us which NaN semantics the reductions are supposed to use...

Diff Detail

Event Timeline

nikic created this revision.Jan 21 2020, 1:00 PM

Do you think this should be a Target independent change, or a change inside shouldExpandReduction for ARM/AArch64? If some target has a reduction that guarantees ordered reductions, they might still want to lower the intrinsic manually. That might all be too hypothetical to worry about thought.

That might all be too hypothetical to worry about thought.

It's not hypothetical; SVE actually has an instruction for ordered reductions (FADDA).

nikic updated this revision to Diff 241010.Jan 28 2020, 2:59 PM

Expand for ARM/AArch64 in shouldExpandReduction() TTI hook.

nikic added a comment.Jan 28 2020, 3:03 PM

I was working off the assumption that no backend could reasonably make use of these ops due to lack of legalization, but I guess it's still possible to use them if you only opt-in to exactly those VTs that you can directly lower. I've now updated the patch to make it ARM/AArch64 specific, via the shouldExpandReductions() TTI hook.

dmgreen accepted this revision.Jan 29 2020, 2:18 PM

Ah, I had presumed that the SVE instructions were pairwise. Nice.

I think we may run into something similar in MVE for nonan min/max reductions, where we do not know how to lower fmaximum. And we may not be testing NoNan flags correctly. I will attempt to go through and make sure we have tests and that it's working OK.

LGTM for this patch. Thanks.

This revision is now accepted and ready to land.Jan 29 2020, 2:18 PM
This revision was automatically updated to reflect the committed changes.