Page MenuHomePhabricator

[RISCV][WIP] Implement intrinsics for P extension
Needs ReviewPublic

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

Details

Summary

It introduces a new macro specified type "e" for function argument.
"e", followed by the base type and its vector type width is the same as the register size,
is used to specify the argument with the certain base type but its element size depends
on XLen. For example, vector type with base type i8 is v4i8 on RV32 and v8i8 on RV64.

It doesn't include sub-extension Zpsfoperand and Zprvsfextra.

Diff Detail

Event Timeline

Jim created this revision.Tue, Mar 23, 2:14 AM
Jim requested review of this revision.Tue, Mar 23, 2:14 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptTue, Mar 23, 2:14 AM
jrtc27 added inline comments.Tue, Mar 23, 7:05 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11170–11175 ↗(On Diff #332560)

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
4044

Upper-case parameter name

clang/test/CodeGen/builtins-riscv-rv32p.c
63 ↗(On Diff #332560)

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

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
778–795

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

llvm/test/CodeGen/RISCV/intrinsics-rv32p.ll
5 ↗(On Diff #332560)

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
769

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

frasercrmck added inline comments.Thu, Mar 25, 3:32 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
778–795

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.Thu, Mar 25, 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.Tue, Apr 6, 12:12 AM
Jim edited the summary of this revision. (Show Details)

Address comments.

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

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.Tue, Apr 6, 12:33 AM
clang/lib/CodeGen/CGBuiltin.cpp
18589

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.Tue, Apr 6, 1:38 AM

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

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

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

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

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

18589

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.

18589

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

llvm/include/llvm/IR/IntrinsicsRISCV.td
1535

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

1535

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

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

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.EditedFri, Apr 16, 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.Fri, Apr 16, 1:08 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
767

I will implement it in another patch for codegen.

craig.topper added inline comments.Fri, Apr 16, 9:26 AM
llvm/test/CodeGen/RISCV/rvp/intrinsics-rv32p.ll
25

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.Fri, Apr 16, 9:41 AM
clang/include/clang/Basic/Builtins.def
41

followd -> followed