This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support scalable-vector integer reduction intrinsics
ClosedPublic

Authored by frasercrmck on Jan 28 2021, 8:02 AM.

Details

Summary

This patch adds support for the integer reduction intrinsics supported
by RVV. This excludes "mul" which has no corresponding instruction.

The reduction instructions in RVV have slightly complicated type
constraints given they always produce a single "M1" vector register.

They are lowered to custom nodes including the second "scalar" reduction
operand to simplify the patterns and in the hope that they can be useful
for future DAG combines.

Diff Detail

Event Timeline

frasercrmck created this revision.Jan 28 2021, 8:02 AM
frasercrmck requested review of this revision.Jan 28 2021, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 8:02 AM
craig.topper added inline comments.Jan 28 2021, 11:34 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1681

Doesn't this need to be the identify value for the operation not always 0? I think AND would need -1. SMIN would need INT_MAX, SMAX would need INT_MIN, UMAX would need UINT_MAX.

1681

Doesn't this need to be the identify value for the operation not always 0? I think AND would need -1. SMIN would need INT_MAX, SMAX would need INT_MIN, UMAX would need UINT_MAX.

Oops UMAX would need 0, UMIN would need UINT_MAX

frasercrmck added inline comments.Jan 29 2021, 2:37 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1681

Wow, of course they would. Can't believe I missed that. Thanks! Must have been too focused on "add".

  • fix identity values for smax,smin,umax,and reductions
  • fix RV32 64-bit reductions using 32-bit identity values
frasercrmck marked 2 inline comments as done.Jan 29 2021, 3:16 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1681

Okay that should be that. I had to ensure it's minIntN, maxIntN, maxUIntN for the element type. The splatting still recognises when it's sign-extended so generates e.g. -1 for the INT_MIN cases.

craig.topper added inline comments.Jan 29 2021, 9:56 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
916

Shouldn't we always lower EXTRACT_ELEMENT for types less than XLen to VMV_X_S? Why is knowing sign extension important for this operation, but not any extract element?

1685

Rename this since its not a Splat of 0 anymore. Also update the commit message.

  • fix variable name
frasercrmck edited the summary of this revision. (Show Details)Jan 29 2021, 10:41 AM
frasercrmck marked an inline comment as done.
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
916

Yes, we really should. I'll put up a patch which addresses that first, then can rebase this on top of that.

1685

Done, thanks for reminding me.

HsiangKai added inline comments.Jan 29 2021, 3:42 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
440

You list ISD::VECREDUCE_MUL here, but you did not handle ISD::VECREDUCE_MUL in LowerOperation.

frasercrmck edited the summary of this revision. (Show Details)Jan 30 2021, 9:27 AM
frasercrmck marked an inline comment as done.
  • rebase on child revision
  • use generic EXTRACT_VECTOR_ELT rather than VMV_S_X
frasercrmck marked 2 inline comments as done.
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
440

Thanks for spotting that. Copy/paste mistake.

frasercrmck marked an inline comment as done.
  • rebase
This revision is now accepted and ready to land.Feb 4 2021, 12:20 PM
This revision was landed with ongoing or failed builds.Feb 5 2021, 2:16 AM
This revision was automatically updated to reflect the committed changes.