Page MenuHomePhabricator

[RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h
Needs ReviewPublic

Authored by s.egerton on Sep 17 2019, 8:15 AM.

Details

Summary

Add header files to Clang containing functions for Bitmanip extension that will emit the correct assembly when the Bitmanip extension is enabled, or if the Bitmanip extension is disabled, provided the same functionality via C code.

Details on the extension can be found here: https://raw.githubusercontent.com/riscv/riscv-bitmanip/master/bitmanip-0.92.pdf

Diff Detail

Event Timeline

s.egerton created this revision.Sep 17 2019, 8:15 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 17 2019, 8:15 AM

Inline asm is really unfriendly to the optimizer.
Ideally the plan should be to incrementally getting rid of it as soon as backend learns to properly match particular builtin.

clang-tools-extra/clang-include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp
59–68

<> missing?

clang-tools-extra/clangd/index/CanonicalIncludes.cpp
110–119

<> missing?

I agree inline asm is a far from optimal solution but it seems like the lesser of two evils for now.

This sounds like a good idea, but we need to be able to guarantee that the backend will be able to match to the correct instruction for all optimisation levels before we can do that.

s.egerton updated this revision to Diff 220647.EditedSep 18 2019, 3:54 AM

Add missing <>

s.egerton marked 2 inline comments as done.Sep 18 2019, 3:55 AM

I agree inline asm is a far from optimal solution but it seems like the lesser of two evils for now.

Hm, i thought some previous patch already adds llvm ir riscv-specific intrinsics for those.

This sounds like a good idea, but we need to be able to guarantee that the backend will be able to match to the correct instruction for all optimisation levels before we can do that.

Sorry I misread your original comment.

These functions exist so that we can guarantee that these particular instructions will be emitted; the other option was LLVM IR intrinsics and Clang builtins, this was the other patch (https://reviews.llvm.org/D66479).

We are planning on abandoning that patch in favour of this one after the discussions on the patch and the mailing list.

Sorry I misread your original comment.

(which one?)

These functions exist so that we can guarantee that these particular instructions will be emitted;

Sure, that makes sense.

the other option was LLVM IR intrinsics and Clang builtins, this was the other patch (https://reviews.llvm.org/D66479).
We are planning on abandoning that patch in favour of this one after the discussions on the patch and the mailing list.

I sure did comment that both of these approaches (emitting inline asm, or having arch-specific intrinsics)
are worse than emitting plain IR (as there is no 'real' incentive to enhance backend pattern-matching),
but arch-specific intrinsics are certainly better than inline asm.
Sorry if that thought got convoluted.

Sorry I misread your original comment.

(which one?)

These functions exist so that we can guarantee that these particular instructions will be emitted;

Sure, that makes sense.

the other option was LLVM IR intrinsics and Clang builtins, this was the other patch (https://reviews.llvm.org/D66479).
We are planning on abandoning that patch in favour of this one after the discussions on the patch and the mailing list.

I sure did comment that both of these approaches (emitting inline asm, or having arch-specific intrinsics)
are worse than emitting plain IR (as there is no 'real' incentive to enhance backend pattern-matching),
but arch-specific intrinsics are certainly better than inline asm.
Sorry if that thought got convoluted.

Err, my main point against arch-specific intrinsics was about NOT producing them in middle-end,
since nothing will know about them, and thus the IR would become more opaque if you produce them in middle-end.

We have a patch to add codegen pattern matching (https://reviews.llvm.org/D67348). Unfortunately we have found that we will not be able to rely on pattern matching here to guarantee that these instructions are emitted in all situations due to differences in optimisation levels and the complexity of some instructions.
The approach of using arch-specific intrinsics in the middle end (https://reviews.llvm.org/D66479) was an alternative way to bridge the gap between the C level builtins and the Bitmanip instructions.

Thinking about it more, this could be approved and eventually have the inline asm parts replaced after the arch-specific intrinsics patch is approved.

s.egerton updated this revision to Diff 221329.Sep 23 2019, 7:49 AM
This comment was removed by s.egerton.
s.egerton updated this revision to Diff 221334.EditedSep 23 2019, 8:01 AM

Fixed typos in opcodes. Missing '.'s were added. e.g. adduw -> addu.w

Removed slliu.w functions. There is no single instruction equivalent for when the second argument to the header function is not an immediate. Additionally there is no way of detecting this from C code and throwing a user friendly error message as __builtin_constant_p is not guaranteed to be reliable across inline boundaries at different optimisation levels.

Also rebased on top of upstream commits.

s.egerton updated this revision to Diff 229831.Nov 18 2019, 6:42 AM
s.egerton edited the summary of this revision. (Show Details)

Update to include changes in v0.92.

Changes in this version:

  • Add packh and packu[w] instructions
  • Add sext.b and sext.h instructions
s.egerton updated this revision to Diff 229857.Nov 18 2019, 8:35 AM

Re-add accidentally reverted update.

The original message was:

Fixed typos in opcodes. Missing '.'s were added. e.g. adduw -> addu.w

Removed slliu.w functions. There is no single instruction equivalent for when the second argument to the header function is not an immediate. Additionally there is no way of detecting this from C code and throwing a user friendly error message as __builtin_constant_p is not guaranteed to be reliable across inline boundaries at different optimisation levels.

Also rebased on top of upstream commits.

So I have a quick comment about this patch, perhaps it might help to get things moving again.

I'd like to see the actual frontend changes, IE separate from the header implementations, to be split into a separate patch. So we can have things like the __riscv_bitmanip macro and the target attribute parsing potentially landed sooner?

lewis-revill added inline comments.Dec 3 2019, 8:15 AM
clang/lib/Headers/rv32bintrin-builtins.h
28

Does GCC perform this check before calling the builtin?

s.egerton updated this revision to Diff 233789.Dec 13 2019, 5:57 AM

Add underscore in place of asm '.' in C function names

For example corresponding function for the asm "addu.w" was previously named "_rv_adduw(...)". Now it is called "_rv_addu_w(...)".

s.egerton planned changes to this revision.Dec 13 2019, 6:07 AM

So I have a quick comment about this patch, perhaps it might help to get things moving again.

I'd like to see the actual frontend changes, IE separate from the header implementations, to be split into a separate patch. So we can have things like the __riscv_bitmanip macro and the target attribute parsing potentially landed sooner?

Sounds good to me. I'll work on separating these out.

s.egerton updated this revision to Diff 234278.EditedDec 17 2019, 5:04 AM
  • Removed frontend changes, as these have been seperated out into a seperate patch here: https://reviews.llvm.org/D71553
  • Made use of __riscv macro in intrin.h, rather than relying on __riscv32 || __riscv64.

Selfishly, I would like to see the addition of rvintrin.h separated from the bit-manipulation-specific headers. I'm looking at landing some additions to clang/LLVM that include builtins, and don't want to cause merge issues with this PR.

I haven't yet had time to fully absorb the bit manipulation spec to ensure that these functions, when emulated, match it.

clang/lib/Headers/rv32bintrin-builtins.h
49

Nit: I don't think this is the case here, based on the calls in the implementation.

khchen added a subscriber: khchen.Dec 6 2020, 11:52 PM
rkruppe removed a subscriber: rkruppe.Dec 7 2020, 8:26 AM
lenary resigned from this revision.Jan 14 2021, 11:07 AM