[PATCH] D155439: [RISCV] Add SDNode patterns for vrol.[vv,vx] and vror.[vv,vx,vi]

[Luke Lau via Phabricator via llvm-commits]

luke added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:3006
 
-bool RISCVDAGToDAGISel::selectVSplatUimm5(SDValue N, SDValue &SplatVal) {
+template <unsigned Bits>
+constexpr inline bool selectVSplatUimm(SDValue N, SDValue &SplatVal,
----------------
craig.topper wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > Don't template this, pass the value as an argument. There show be a form of `isUInt` that takes an argument.
> > "show" was supposed to be "should"
> You can make a remplate version of `selectVSplatUimm` that takes a template parameter and forwards the argument as a non-template parameter. 
> 
> See for example
> 
> ```
>   bool selectSExtBits(SDValue N, unsigned Bits, SDValue &Val);                   
>   template <unsigned Bits> bool selectSExtBits(SDValue N, SDValue &Val) {        
>     return selectSExtBits(N, Bits, Val);                                         
>   }   
> ```
> 
> Basically I'm trying to avoid the binary bloat from duplicating a function that barely cares about the number of bits.
Yeah that's much better, done and will include in the next diff


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vror-sdnode.ll:85
+; CHECK-ZVBB-NEXT:    vsetvli a0, zero, e8, mf8, ta, ma
+; CHECK-ZVBB-NEXT:    vror.vi v8, v8, -1
+; CHECK-ZVBB-NEXT:    ret
----------------
craig.topper wrote:
> I suspect the assembler won't parse this.
Your suspicion is correct. The verifier doesn't seem to like 64 - uimm either:

```
*** Bad machine code: Invalid immediate ***
- function:    vror_vi_rotl_nxv1i8
- basic block: %bb.0  (0x15b0a5970)
- instruction: %1:vr = PseudoVROR_VI_MF8 %2:vr(tied-def 0), %0:vr, 63, -1, 3, 3
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155439