Page MenuHomePhabricator

[RISCV] [1/2] Add intrinsic for Zbb extension
ClosedPublic

Authored by LevyHsu on Thu, Mar 25, 12:22 AM.

Details

Summary

Head files are included in the second patch in case the name needs to be changed.

RV32 / 64:
orc.b

__builtin_riscv_orc_b_32
__builtin_riscv_orc_b_64

_rv_orc_b
_rv32_orc_b
_rv64_orc_b

Diff Detail

Event Timeline

LevyHsu created this revision.Thu, Mar 25, 12:22 AM
LevyHsu requested review of this revision.Thu, Mar 25, 12:22 AM

Please upload the patch using arcanist or using -U999999 when generating the diff.

LevyHsu edited the summary of this revision. (Show Details)Mon, Mar 29, 2:14 AM
LevyHsu updated this revision to Diff 333798.EditedMon, Mar 29, 3:11 AM
  1. Generated with git format-patch -o patches/ -2 HEAD -U999999
  2. clang/lib/Sema/SemaChecking.cpp
    • Rewrote CheckRISCVBuiltinFunctionCall
  3. clang/lib/CodeGen/CGBuiltin.cpp
    • IntrinsicTypes = {ResultType};
  4. llvm/include/llvm/IR/IntrinsicsRISCV.td
    • The second llvm_any_ty has been changed to LLVMMatchType<0>
  5. llvm/lib/Target/RISCV/RISCVISelLowering.cpp
    • ANY_EXTEND both op0 and op 0 then truncate back to i32 when orc32b under RV64
  6. llvm/test/CodeGen/RISCV/rv32zbb-intrinsic.ll llvm/test/CodeGen/RISCV/rv64zbb-intrinsic.ll clang/test/CodeGen/RISCV/rvb-intrinsics/riscv32-zbb.c clang/test/CodeGen/RISCV/rvb-intrinsics/riscv64-zbb.c
    • Added corresponding testcases.
LevyHsu edited the summary of this revision. (Show Details)Mon, Mar 29, 3:11 AM
craig.topper added inline comments.Mon, Mar 29, 10:57 AM
clang/include/clang/Basic/BuiltinsRISCV.def
21

Can we select between the 32 and 64 bit version in the header based on __riscv_xlen so we only need 2 builtins, rather than 3?

22

All RISCV builtins should start with "__builtin_riscv_". I don't think we should use riscv32/riscv64 here. Especially since the 32-bit version is available on RV64. So I think these should be named

builtin_riscv_orc_b_32 and builtin_riscv_orc_b_64.

23

Ideally this would be "experimental-zbb,64bit" but I'm not sure that the frontend knows about "64bit".

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

"LHS" stands for "left hand side", but operand 0 is the intrinsic ID which should already be i64. Only operand 1 needs to be extended. So just do

SDValue NewOp1 = DAG.getNode(ISD::ANY_EXTEND, DL, MVT::i64, N->getOperand(1));
SDValue Res = DAG.getNode(N->getOpcode(), DL, MVT::i64, N->getOperand(0), NewOp1);
LevyHsu updated this revision to Diff 334072.Tue, Mar 30, 1:40 AM
  1. llvm/lib/Target/RISCV/RISCVISelLowering.cpp
    • Fixed mishandling on op0
  2. clang/include/clang/Basic/BuiltinsRISCV.def clang/lib/CodeGen/CGBuiltin.cpp
    • Reduce port to 2 versions for 32/64 only.
  3. clang/test/CodeGen/RISCV/rvb-intrinsics/riscv32-zbb.c clang/test/CodeGen/RISCV/rvb-intrinsics/riscv64-zbb.c
    • Remove extra tests
craig.topper added inline comments.Tue, Mar 30, 11:02 AM
clang/lib/Sema/SemaChecking.cpp
3418

LLVM coding style does not allow _ in variable names.

3427

Drop the else. Since the if has a continue, control flow won't reach here if the 'if' is true.

clang/test/CodeGen/RISCV/rvb-intrinsics/riscv64-zbb.c
26

Why _2?

LevyHsu updated this revision to Diff 334322.Tue, Mar 30, 7:06 PM
  1. clang/lib/Sema/SemaChecking.cpp
    • Fixed var name & loop
  2. clang/test/CodeGen/RISCV/rvb-intrinsics/riscv64-zbb.c
    • renamed function

I think this looks good to me. Anyone else have any comments?

Jim added inline comments.Wed, Mar 31, 12:07 AM
llvm/include/llvm/IR/IntrinsicsRISCV.td
26

How about put it between Atomics and Vector to follow canonical order?

asb added a comment.EditedWed, Mar 31, 6:16 AM

Can I just check the reasoning on the naming? I see that the bitmanip 0.93 spec proposes _{rv,rv32,rv64}_{opname} intrinsics. Does the __builtin__{riscv,riscv32,riscv64}_opname format match what GCC are doing / planning to do here? Precedent for RVV, for other archs, or something else?

Just trying to determine to what these names are an open item of discussion vs matching something else.

EDIT: I see Craig commented on this point in https://reviews.llvm.org/D99009#2660456

craig.topper edited the summary of this revision. (Show Details)Wed, Mar 31, 9:11 AM
craig.topper added a comment.EditedWed, Mar 31, 9:18 AM
In D99320#2661285, @asb wrote:

Can I just check the reasoning on the naming? I see that the bitmanip 0.93 spec proposes _{rv,rv32,rv64}_{opname} intrinsics. Does the __builtin__{riscv,riscv32,riscv64}_opname format match what GCC are doing / planning to do here? Precedent for RVV, for other archs, or something else?

Just trying to determine to what these names are an open item of discussion vs matching something else.

EDIT: I see Craig commented on this point in https://reviews.llvm.org/D99009#2660456

Yeah the big open for naming is the name of the intrinsic header. Whether it should be rvintrin.h or riscv_intrinsic.h.

The builtins in this patch are

builtin_riscv_orc_b_32
builtin_riscv_orc_b_64

I've update the description summary to reflect this since it change during development.

The Zbr patch uses the following without a 32/64 suffix.
__builtin_riscv_crc32_b/w/d/q

There's a difference because orc.b needs to be available for 32-bit on RV32 and RV64 per the spec so we have two builtins. crc32.b intrinsics in the spec are defined only for xlen so we have a single builtin.

@kito-cheng do you know how gcc will name builtins?

Created an issue for continue discuses on riscv-c-api-doc
https://github.com/riscv/riscv-c-api-doc/issues/19

craig.topper accepted this revision.Thu, Apr 1, 11:00 PM

I'm going to approve this. If we need to change the builtin names in the future that's easy enough to do.

This revision is now accepted and ready to land.Thu, Apr 1, 11:00 PM
This revision was landed with ongoing or failed builds.Fri, Apr 2, 11:52 AM
This revision was automatically updated to reflect the committed changes.