[RISCV] [1/2] Add intrinsic for Zbr extension

[LevyHsu]

Summary

Implementation for RISC-V Zbr extension intrinsic.

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

RV32 / 64:

crc32b
crc32h
crc32w
crc32cb
crc32ch
crc32cw

RV64 Only:

crc32d
crc32cd

[jrtc27]

Please fix the style issues and update the patch with full context (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface, or use arc)

clang/include/clang/Basic/BuiltinsRISCV.def

20

Zbr

clang/include/clang/Basic/DiagnosticSemaKinds.td

11170

This will apply to non-experimental extensions too once V/B/K/etc get ratified

clang/lib/Sema/SemaChecking.cpp

3400

This new code assumes there's only one feature in the string

3405

Extension name should have an upper case first letter

clang/test/CodeGen/RISCV/rvb-intrinsics/riscv32-zbr.c

16–17

long crc32b(long a) { for all these

clang/test/Headers/rvintrin.c

5 ↗

(On Diff #332079)

This does nothing unless you add -verify to the Clang command line

llvm/include/llvm/IR/IntrinsicsRISCV.td

20

Space after comma

llvm/lib/Target/RISCV/RISCVInstrInfoB.td

879

Space after comma

llvm/test/CodeGen/RISCV/rv32Zbr.ll

1 ↗

(On Diff #332079)

Lowercase z in the names of these files as it's a (sort of) -march string

[craig.topper]

clang/test/CodeGen/RISCV/rvb-intrinsics/riscv32-zbr.c

6

I don't think this include is needed. You're calling the __builtin names directly.

clang/test/CodeGen/RISCV/rvb-intrinsics/riscv64-zbr.c

6

Same here

clang/test/Headers/rvintrin.c

7 ↗

(On Diff #332079)

This should be in the D99008 patch

[craig.topper]

llvm/test/CodeGen/RISCV/rv32Zbr.ll

1 ↗

(On Diff #332079)

The bitmanip tests were already have all us "Zb*" in their file names. Should we match that or rename the existing tests?

[jrtc27]

llvm/test/CodeGen/RISCV/rv32Zbr.ll

1 ↗

(On Diff #332079)

:( ideally they'd be renamed IMO, but if not then yeah I guess this should stay as it is to match

[LevyHsu]

1. Fix format issue
2. SemaChecking.cpp should allow Diag to print all missing features now.

[craig.topper]

Please reupload the patch using something like "git diff -U99999". We should be able to expand the full diff here so that phabricator doesn't say "Context not available."

[craig.topper]

clang/lib/Sema/SemaChecking.cpp

3398

Variable names should start with a capital letter and use CamelCase.

3401

Features is already a StringRef, no need to cast it before calling split.

3404

Use a range-based for loop.

3405

Invert the condition and use a continue; Then we can reduce nesting for the rest of the code.

3415

Please fix formatting as the Pre-merge check suggests.

llvm/include/llvm/IR/IntrinsicsRISCV.td

29

Put crc32_d directly below crc32_w. So all the crc32_ are together and all the crc32c_ are together.

[craig.topper]

clang/lib/CodeGen/CGBuiltin.cpp

17865

With the llvm_any_ty change, you'll only need ResultType here.

llvm/include/llvm/IR/IntrinsicsRISCV.td

20

The second llvm_any_ty here should be LLVMMatchType<0> since the types need to be the same.

[LevyHsu]

1. clang/lib/Sema/SemaChecking.cpp
   • Rewrite Sema::CheckRISCVBuiltinFunctionCall
   • Coding style fix
2. llvm/include/llvm/IR/IntrinsicsRISCV.td
   • BitMan_GPR_Intrinsics's op1 now matches type of op0
   • Put crc32_d directly below crc32_w.
   • Coding style fix
3. llvm/lib/Target/RISCV/RISCVInstrInfoB.td
   • Coding style fix

[Jim]

clang/include/clang/Basic/BuiltinsRISCV.def

28

I don't know why it doesn't use rv* prefix directly?

[craig.topper]

clang/include/clang/Basic/BuiltinsRISCV.def

28

I believe using builtin* is consistent with how intrinsics are handled on most targets in both clang and gcc. We wouldn't want the rv symbols declared unless the user has included rvintrin.h. There is some support for detecting that, see HEADER_BUILTIN in Builtins.def, but it's used rarely for target specific intrinsics.

[craig.topper]

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

[craig.topper]

LGTM. We can adjust the builtin names in the future if we need to do something different to match gcc.

[jrtc27]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11190

This lost the quotes

clang/lib/Sema/SemaChecking.cpp

3427

both to improve the English and to be clear that this is for non-experimental feature strings too

llvm/include/llvm/IR/IntrinsicsRISCV.td

19

This file, like the rest of LLVM, uses a 2-space indent, not 4.

[craig.topper]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11190

I will fix when I rebase Zbb on top of this

clang/lib/Sema/SemaChecking.cpp

3427

I will fix when I rebase Zbb on top of this

llvm/include/llvm/IR/IntrinsicsRISCV.td

19

I will fix when I rebase Zbb on top of this. I'll also move it below atomics.

[jrtc27]

Thanks, and good point re ordering

[thakis]

Looks like this doesn't build on windows: http://45.33.8.238/win/36271/step_4.txt

Please take a look and revert for now if it takes a while to fix.

[jrtc27]

In D99009#2666793, @thakis wrote:

Looks like this doesn't build on windows: http://45.33.8.238/win/36271/step_4.txt

Please take a look and revert for now if it takes a while to fix.

Hopefully fixed in 1bd4986e7cdc

[thakis]

In D99009#2666797, @jrtc27 wrote:

In D99009#2666793, @thakis wrote:

Looks like this doesn't build on windows: http://45.33.8.238/win/36271/step_4.txt

Please take a look and revert for now if it takes a while to fix.

Hopefully fixed in 1bd4986e7cdc

Yup, better now. Thanks for the fix!