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

[Sam Elliott via Phabricator via cfe-commits]

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 fully absorb the bit manipulation spec to ensure that 
these functions, when emulated, match it.




Comment at: clang/lib/Headers/rv32bintrin-builtins.h:48
+// Genric aliases
+// e.g. _rv_* is an alias of _rv64_*
+

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Scott Egerton via Phabricator via cfe-commits]

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 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lewis Revill via Phabricator via cfe-commits]

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 builtin?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lewis Revill via Phabricator via cfe-commits]

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 __riscv_bitmanip macro and the target attribute parsing potentially landed 
sooner?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Scott Egerton via Phabricator via cfe-commits]

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 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

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.

> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

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 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Scott Egerton via Phabricator via cfe-commits]

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 planning on abandoning that patch in favour of this one after the 
discussions on the patch and the mailing list.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

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 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.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Scott Egerton via Phabricator via cfe-commits]

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 before we can do that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

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: 
clang-tools-extra/clang-include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp:59-68
+  {"include/rv32bintrin-builtins.h$", "rv32bintrin-builtins.h"},
+  {"include/rv32bintrin-emulation.h$", "rv32bintrin-emulation.h"},
+  {"include/rv32bintrin.h$", "rv32bintrin.h"},
+  {"include/rv64bintrin-asm.h$", "rv64bintrin-asm.h"},
+  {"include/rv64bintrin-builtins.h$", "rv64bintrin-builtins.h"},
+  {"include/rv64bintrin-emulation.h$", "rv64bintrin-emulation.h"},
+  {"include/rv64bintrin.h$", "rv64bintrin.h"},

`<>` missing?



Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:157-166
+  {"include/rv32bintrin-builtins.h", "rv32bintrin-builtins.h"},
+  {"include/rv32bintrin-emulation.h", "rv32bintrin-emulation.h"},
+  {"include/rv32bintrin.h", "rv32bintrin.h"},
+  {"include/rv64bintrin-asm.h", "rv64bintrin-asm.h"},
+  {"include/rv64bintrin-builtins.h", "rv64bintrin-builtins.h"},
+  {"include/rv64bintrin-emulation.h", "rv64bintrin-emulation.h"},
+  {"include/rv64bintrin.h", "rv64bintrin.h"},

`<>` missing?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67661/new/

https://reviews.llvm.org/D67661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits