This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support the scalable-vector fadd reduction intrinsic
ClosedPublic

Authored by frasercrmck on Feb 2 2021, 7:15 AM.

Details

Summary

This patch adds support for both the fadd reduction intrinsic, in both
the ordered and unordered modes.

The fmin and fmax intrinsics are not currently supported due to a
discrepancy between the LLVM semantics and the RVV ISA behaviour with
regards to signaling NaNs. This behaviour is likely fixed in version 2.3
of the RISC-V F/D/Q extension, but until then the intrinsics can be left
unsupported.

Diff Detail

Event Timeline

frasercrmck created this revision.Feb 2 2021, 7:15 AM
frasercrmck requested review of this revision.Feb 2 2021, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2021, 7:15 AM
craig.topper added inline comments.Feb 2 2021, 9:28 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1718

Won't getSimpleVT) below already assert?

1719

getSimpleValueType()

1731

I think ISD::SCALAR_TO_VECTOR is what we would more commonly use.

llvm/test/CodeGen/RISCV/rvv/vreductions-fp-sdnode.ll
31

Annoying that SelectionDAG breaks the scalar input out separately.

jrtc27 added a comment.Feb 2 2021, 9:28 AM

If something's not yet supported due to a spec deficiency that should be documented in the code with a link to the outstanding spec issue.

craig.topper added inline comments.Feb 2 2021, 7:42 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1731

i guess we don't have a true scalar_to_vector instruction in RVV, i.e. an instruction that just moves a scalar register without being dependent on the previous vector register. So I think INSERT_VECTOR_ELT is ok.

If something's not yet supported due to a spec deficiency that should be documented in the code with a link to the outstanding spec issue.

That's a good idea. My first thought would be RISCVTTIImpl::shouldExpandReduction, not that it can actually expand it right now? Regardless of our intentions, if we find such a reduction in the IR we crash.

  • simplify SimpleValueType access
  • update documentation about missing fmin/fmax reductions
frasercrmck marked 4 inline comments as done.Feb 3 2021, 3:50 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1718

Yep, thanks. Done as part of using getSimpleValueType() below.

1731

Yeah, tbh, I can see how it could go either way. Since the upper elements of SCALAR_TO_VECTOR are undefined I suppose we could define it as equivalent to a splat. But I don't think that's worth doing, and INSERT_VECTOR_ELT better matches the semantics of the "scalar" operands of these reductions.

frasercrmck marked 2 inline comments as done.
  • rebase
This revision is now accepted and ready to land.Feb 4 2021, 11:59 AM
craig.topper requested changes to this revision.Feb 4 2021, 12:15 PM
craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1731

Should we splat here like we do for the integer reductions?

This revision now requires changes to proceed.Feb 4 2021, 12:15 PM
craig.topper added inline comments.Feb 4 2021, 12:16 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1731

I think we have a special case for splatting 0.0 so that could be beneficial.

  • address Craig's suggestions to use a splat instead of insertelt0
  • could perhaps merge the int and fp reduction lowering code if we wanted to,

though it's not a perfect fit

This revision is now accepted and ready to land.Feb 5 2021, 9:32 AM
This revision was landed with ongoing or failed builds.Feb 8 2021, 1:58 AM
This revision was automatically updated to reflect the committed changes.