[PATCH] D91259: [RISCV] Lower GREVI and GORCI as custom nodes

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1153
+// Match the following pattern as a GORCI(W) operation
+//   (or (or (BITMANIP_SHL x), x),
+//       (BITMANIP_SRL x))
----------------
frasercrmck wrote:
> craig.topper wrote:
> > frasercrmck wrote:
> > > craig.topper wrote:
> > > > Is it possible that the input is (or (or (BITMANIP_SHL x), (BITMANIP_SHR x)), x)?
> > > Yeah. It's currently generating this on RV32 master:
> > > 
> > >  ```
> > >   %shl = shl i32 %a, 16
> > >   %shr = lshr i32 %a, 16
> > >   %or = or i32 %shr, %shl
> > >   %or2 = or i32 %or, %a
> > >   ret i32 %or2
> > > 
> > > -->
> > >   rev16   a1, a0
> > >   or      a0, a1, a0
> > > ```
> > > 
> > > In that case it's essentially `or (grevi x), x)` so could in theory be picked up like that rather than fully exploring all potential combinations of `(or (or x, y), (or z, w))` inputs.
> > > 
> > > Sadly though LLVM is picking up that `shl/srl` as `rotl`, and still being TableGen pattern matched to grev.
> > > 
> > > That relates to my other comment, that we might need to dive further into `rotl`, `rotr`, `bswap` and `bitreverse` to find more opportunities.
> > Can you post your patch to a separate review? I'd like to experiment with it.
> In the absence of time I just sent you an email with the wip patch
My understanding of what you've done in D91449 would be that it would help us more easily find `(or (rorw x), x)` and generate a `gorc`. I think what you've done in removing the reliance on `SIGN_EXTEND_INREG` is taking our support in a positive direction.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1138
+                               const RISCVSubtarget &Subtarget) {
+  if (Op.getSimpleValueType() == Subtarget.getXLenVT()) {
+    auto LHS = matchRISCVBitmanipPat(Op.getOperand(0), IsW);
----------------
craig.topper wrote:
> frasercrmck wrote:
> > frasercrmck wrote:
> > > craig.topper wrote:
> > > > Could we match i32 pre-type legalization on RV64 and type legalize i32 RISCVISD::GREVI to GREVIW? I think that would remove the need to match from SIGN_EXTEND_INREG.
> > > Good idea; I'll give it a go.
> > This idea mostly works but is failing for two RV64 test cases: i32 `fshl` and `fshr` intrinsics with constant 16.
> > 
> > `fshl` is lowered to sign-extended i32 `rotl` instructions and dag-combined to a `(sext_inreg (i64 (or (shl x, 16), (srl (and x, 0xffff0000), 16))))` pattern, skipping the 32-bit or combine. That's something the sext combine was getting but not something the or combine is. Similar for `fshr`. I think we need the sext in there to be able to match this as a greviw.
> > 
> > Combining certain `rotl` (and `rotr`, `bswap` and `bitreverse`) to grevi would catch this and would unify everything under one node (something I was mulling about anyway as it'd provide more opportunity for grev combining) but it might also be heavy-handed.
> > This idea mostly works but is failing for two RV64 test cases: i32 `fshl` and `fshr` intrinsics with constant 16.
> > 
> > `fshl` is lowered to sign-extended i32 `rotl` instructions and dag-combined to a `(sext_inreg (i64 (or (shl x, 16), (srl (and x, 0xffff0000), 16))))` pattern, skipping the 32-bit or combine. That's something the sext combine was getting but not something the or combine is. Similar for `fshr`. I think we need the sext in there to be able to match this as a greviw.
> > 
> > Combining certain `rotl` (and `rotr`, `bswap` and `bitreverse`) to grevi would catch this and would unify everything under one node (something I was mulling about anyway as it'd provide more opportunity for grev combining) but it might also be heavy-handed.
> 
> I'm not sure that we should be using GREVI for cases that can be handled with ROLW/RORW. I could see a CPU having a better performing implementation of rotate than grevi.
Fair enough. Do you think D91449 and D91479 address this problem by catching `fshl`, `fshr`, `rotl` and `rotr` earlier?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91259