[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

lenary added a comment. 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

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

s.egerton planned changes to this revision. s.egerton added a comment. In D67661#1767141 , @lewis-revill wrote: > 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

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

lewis-revill added inline comments. Comment at: clang/lib/Headers/rv32bintrin-builtins.h:27 +_rv32_clz(const uint_xlen_t rs1) { + // Calling these builtins with 0 results in undefined behaviour. + if (rs1 == 0) { Does GCC perform this check before calling the

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

lewis-revill added a comment. 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

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

s.egerton added a comment. 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

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

lebedev.ri added a comment. In D67661#1673918 , @s.egerton wrote: > 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. >

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

lebedev.ri added a comment. In D67661#1673993 , @lebedev.ri wrote: > In D67661#1673918 , @s.egerton wrote: > > > Sorry I misread your original comment. > > > (which one?) > > > These functions exist so that we can

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

s.egerton added a comment. 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

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

lebedev.ri added a comment. In D67661#1673712 , @s.egerton wrote: > 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

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

s.egerton added a comment. 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

[PATCH] D67661: [RISCV] Headers: Add Bitmanip extension Clang header files and rvintrin.h

lebedev.ri added a comment. 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. Comment at: