This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Implement intrinsics for P extension
Needs ReviewPublic

Authored by Jim on Mar 23 2021, 2:14 AM.

Details

Summary

Support for P extension version 0.9.11

Diff Detail

Event Timeline

Jim created this revision.Mar 23 2021, 2:14 AM
Jim requested review of this revision.Mar 23 2021, 2:14 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 23 2021, 2:14 AM
jrtc27 added inline comments.Mar 23 2021, 7:05 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11170–11175

I seem to recall another patch that includes generalising this to take the extension name rather than adding multiple copies of the same thing

clang/lib/AST/ASTContext.cpp
4037

Upper-case parameter name

clang/test/CodeGen/builtins-riscv-rv32p.c
63

One function per test and update_cc_test_checks.py would be a lot nicer IMO

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
715–732

This seems like it will interact poorly with V if both are present

llvm/test/CodeGen/RISCV/intrinsics-rv32p.ll
5

This is an awful test. One function per intrinsic, and no unnecessary alloca's.

What are we gaining from making the intrinsics use vector types if no vector operations are supported other than the intrinsics? Why can't we just use an xlen integer type?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
706

What about bitcast from float/double to any of these vector types? I'm guess that's not legal.

frasercrmck added inline comments.Mar 25 2021, 3:32 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
715–732

Indeed. I don't think we've really thought about how both of these could co-exist in llvm. I'm not sure they can. We don't have the context to decide upon how to lower an operation (to V or P) since we're just given the type.

Do we have to error if both are enabled simultaneously? Maybe if P only expects to be lowered via intrinsics we can fudge it. It's not pretty though. I came across this issue in a previous architecture where we had a v2i32 (or something) able to be lowered both via "scalar" and "vector" instructions.

Jim added a comment.Mar 25 2021, 3:51 AM

What are we gaining from making the intrinsics use vector types if no vector operations are supported other than the intrinsics? Why can't we just use an xlen integer type?

It can support other operations. I will upload a patch for codegen pattern later.

Jim updated this revision to Diff 335416.Apr 6 2021, 12:12 AM
Jim edited the summary of this revision. (Show Details)

Address comments.

Jim marked 3 inline comments as done.Apr 6 2021, 12:14 AM
Jim added inline comments.Apr 6 2021, 12:21 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
706

Yes, it is not legal bitcast from float/double to any of these vector types.
All operations of these vector types have been expanded on line 760.
Only bitcast from/to v4i8/v2i16, v8i8/v4i16/v2i32 to/from i32, i64 are legal.

Jim added inline comments.Apr 6 2021, 12:33 AM
clang/lib/CodeGen/CGBuiltin.cpp
17862

I have concern here. It has lots of duplicate code if the code style is the same as B in the top.
And the name of macro is not good.

Jim updated this revision to Diff 335428.Apr 6 2021, 1:38 AM

Subtarget.hasStdExtP() -> Subtarget.hasStdExtZpn() for addRegisterClass

Jim added inline comments.Apr 6 2021, 1:50 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
706

Oh, it is a issue. I will try to fix it.

craig.topper added inline comments.Apr 6 2021, 9:48 AM
clang/lib/CodeGen/CGBuiltin.cpp
17862

Each assignment to IntrinsicTypes invokes the SmallVector constructor so each one generates a call in the final binary unless the compiler does a good job of merging them.

17862

Please put this above the vector extension since the comment says "vector builtins are handled from here"

18117

Please undef the macros when you're done with them.

llvm/include/llvm/IR/IntrinsicsRISCV.td
1160

I haven't followed the P extension closely. Why do we need 2 kinds of intrinsics for most instructions?

1160

RISCVUnary is a generic name, but the scalar and vector intrinsic inside make it P extension specific.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
704

You probably need handling for insert_vector_elt and extract_vector_elt or some of the expansions will probably break.

Jim updated this revision to Diff 338017.EditedApr 16 2021, 12:57 AM

Only define 1 kind of intrinsic for most intructions, which
operand type and result type are the same scalar or vector type.
But s[z]unpkd* instructions have two kinds of intrinsics, extra
one has operand element width is half of result element width but
with more elements.

Refine code for IR codegen in CGBuiltin.cpp to look like B extension's implementation.

Jim added inline comments.Apr 16 2021, 1:08 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
704

I will implement it in another patch for codegen.

craig.topper added inline comments.Apr 16 2021, 9:26 AM
llvm/test/CodeGen/RISCV/rvp/intrinsics-rv32p.ll
25 ↗(On Diff #338017)

I'm still not clear why we need to have two different ways to do the same operation. Why isn't the C interface all scalar types or all vector types? How do users choose which interface to use?

Also can you please explain the vector codegen plan at a high level? Do you intend to support auto vectorization or just using vector_size in C?

craig.topper added inline comments.Apr 16 2021, 9:41 AM
clang/include/clang/Basic/Builtins.def
41

followd -> followed

Jim updated this revision to Diff 338414.Apr 18 2021, 7:30 PM

Fix typo.

Jim added a comment.Apr 18 2021, 7:36 PM

Also can you please explain the vector codegen plan at a high level? Do you intend to support auto vectorization or just using vector_size in C?

Currently, it just supports vector type operation (like v4i8+v4i8=>add8) in my local patch.

llvm/test/CodeGen/RISCV/rvp/intrinsics-rv32p.ll
25 ↗(On Diff #338017)

I will ask P extension intrinsic designer about your concern.

Jim updated this revision to Diff 345342.May 13 2021, 8:28 PM

Remove intrinsic function with vector type

Jim updated this revision to Diff 349823.Jun 4 2021, 4:57 AM

Rebase

Jim updated this revision to Diff 350094.Jun 6 2021, 3:50 AM

hasStdExtP -> hasStdExtZpn

Jim updated this revision to Diff 352109.Jun 15 2021, 6:02 AM

Alias swap16 to pkbt16.

Jim updated this revision to Diff 370175.Sep 2 2021, 12:32 AM

Rebase and remove unused macro.

Jim edited the summary of this revision. (Show Details)Sep 2 2021, 12:44 AM
Jim updated this revision to Diff 370844.Sep 5 2021, 10:17 PM

Remove extra spaces

Jim updated this revision to Diff 372059.Sep 11 2021, 12:04 AM

Update to P extension spec 0.96

  1. Remove CLO intrinsics
  2. Change intrinsic preifx from rv to __rv_
Jim updated this revision to Diff 372062.Sep 11 2021, 1:27 AM

Add intrinsic code generation for klli8, kslli16 and kslliw.

Jim updated this revision to Diff 372064.Sep 11 2021, 1:59 AM

Add prefix Int into intrinsic Pat

sunshaoce updated this revision to Diff 441928.Jul 3 2022, 2:00 AM
sunshaoce added a subscriber: sunshaoce.

Support for P extension version 0.9.11

sunshaoce retitled this revision from [RISCV][WIP] Implement intrinsics for P extension to [RISCV] Implement intrinsics for P extension.Jul 3 2022, 2:02 AM
sunshaoce edited the summary of this revision. (Show Details)
llvm/test/CodeGen/RISCV/intrinsics-rv32p.ll