Page MenuHomePhabricator

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

Authored by LevyHsu on Mar 19 2021, 10:22 PM.

Details

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

Diff Detail

Event Timeline

LevyHsu created this revision.Mar 19 2021, 10:22 PM
LevyHsu requested review of this revision.Mar 19 2021, 10:22 PM

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 added inline comments.Mar 22 2021, 9:22 AM
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 added inline comments.Mar 22 2021, 6:09 PM
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 added inline comments.Mar 22 2021, 6:16 PM
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 updated this revision to Diff 332562.EditedMar 23 2021, 1:51 AM
LevyHsu edited the summary of this revision. (Show Details)
  1. Fix format issue
  2. SemaChecking.cpp should allow Diag to print all missing features now.
LevyHsu edited the summary of this revision. (Show Details)Mar 25 2021, 12:17 AM

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 added inline comments.Mar 25 2021, 4:39 PM
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 added inline comments.Mar 28 2021, 8:17 PM
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 updated this revision to Diff 334331.Mar 30 2021, 8:14 PM
LevyHsu marked 9 inline comments as done.
  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 added inline comments.Mar 30 2021, 8:30 PM
clang/include/clang/Basic/BuiltinsRISCV.def
28

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

craig.topper added inline comments.Mar 30 2021, 8:46 PM
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.

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

craig.topper accepted this revision.Apr 1 2021, 11:02 PM

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

This revision is now accepted and ready to land.Apr 1 2021, 11:02 PM
This revision was landed with ongoing or failed builds.Apr 2 2021, 11:02 AM
This revision was automatically updated to reflect the committed changes.
jrtc27 added inline comments.Apr 2 2021, 11:08 AM
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 added inline comments.Apr 2 2021, 11:18 AM
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.

Thanks, and good point re ordering

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.

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

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!