[PATCH] D66479: [RISCV] Add LLVM intrinsics for the Bit Manipulation extension

[Scott Egerton via Phabricator via llvm-commits]

s.egerton marked an inline comment as done.
s.egerton added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:99
+
+def int_riscv_bmatflip : BitmanipGprIntrinsic;
+
----------------
lebedev.ri wrote:
> jeremybennett wrote:
> > spatel wrote:
> > > xbolva00 wrote:
> > > > s.egerton wrote:
> > > > > I haven't added intrinsics for every new instruction. Either because they are too simple (andn, orn, xnor, min and max) or because there is already an IR intrinsic that has the same function (clz, ctz, pcnt, ror, rol, fsl and fsr).
> > > > > Are there any other IR intrinsics that it would be better to remove?
> > > > InstCombine could benefit from smin/smax/umin/umax intrinsic
> > > > 
> > > > @spatel ?
> > > I agree, and I raised that possibility ~3 years ago (and it was debated well before that time too):
> > > http://lists.llvm.org/pipermail/llvm-dev/2016-November/106868.html
> > > ...but the consensus was that we could pattern match our way out of it (and adjust the cost models to deal with it), so the benefit of adding intrinsics was not worth the cost. 
> > > 
> > > If there's new evidence to suggest we should have those intrinsics, please send a note to llvm-dev. Then, it can be seen by more people. It's not clear to me how this patch review would be changed.
> > I note that GCC support for RISC-V bit manipulation extension defines a set of builtins (not yet upstream, available at https://github.com/riscv/riscv-bitmanip/blob/master/cproofs/rvintrin.h), and that this set of builtins has been agreed with Clifford Wolf, chair of the Bit Manipulation Task Group and author of the instruction set extension definition document.
> > 
> > Clang/LLVM should support this set as a minimum in order to ensure that code that works on GCC works out of the box on Clang/LLVM.
> A frontend(clang) intrinsic, LLVM IR instruction/intrinsic, backend RISCVISD node and finally RISCV instruction
> are **not** strongly interdependant.
> 
> In order to have a frontend(clang) intrinsic/builtin, you don't need anything from the rest of the compiler,
> you can either just write it in C, or add a builtin to clang that will emit some appropriate LLVM IR.
> 
> You don't need to have any specific LLVM IR instruction/intrinsic for every ISA extension, 
> that would be extremely intrusive; you at most want an RISCVISD node, and patternmatch it from
> normal ISD nodes in RISCVISelLowering.cpp
> 
You raise a good point.
This has been discussed and we have decided to revisit our implementation strategy. There will be future patches to support this.

For future reference: Here is a link to similar discussions on the mailing list: https://lists.llvm.org/pipermail/llvm-dev/2019-August/134791.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66479