This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add SDNode patterns for vrol.[vv,vx] and vror.[vv,vx,vi]
ClosedPublic

Authored by luke on Jul 17 2023, 4:15 AM.

Diff Detail

Unit TestsFailed

Event Timeline

luke created this revision.Jul 17 2023, 4:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 4:15 AM
luke requested review of this revision.Jul 17 2023, 4:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 4:15 AM
craig.topper added inline comments.Jul 17 2023, 8:27 AM
llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
257

What about rotl by immediate using vror.vi with SEW-rotateamt?

258

vror has a uimm6.

luke updated this revision to Diff 541134.Jul 17 2023, 10:44 AM

Add patterns for rotl using vror.vi with negated immediate
Use uimm6

craig.topper added inline comments.Jul 17 2023, 11:01 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3006

Don't template this, pass the value as an argument. There show be a form of isUInt that takes an argument.

craig.topper added inline comments.Jul 17 2023, 11:15 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3006

"show" was supposed to be "should"

craig.topper added inline comments.Jul 17 2023, 11:18 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3006

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.

craig.topper added inline comments.Jul 17 2023, 12:42 PM
llvm/test/CodeGen/RISCV/rvv/vror-sdnode.ll
86

I suspect the assembler won't parse this.

luke added inline comments.Jul 18 2023, 5:30 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3006

Yeah that's much better, done and will include in the next diff

llvm/test/CodeGen/RISCV/rvv/vror-sdnode.ll
86

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
craig.topper added inline comments.Jul 18 2023, 4:28 PM
llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
204

Need to pass <uimm6> to VPseudoVALU_VV_VX_VI

luke updated this revision to Diff 541880.Jul 19 2023, 1:25 AM

Remove template in selectVSplatUImm and fix immediate being signed

craig.topper added inline comments.Jul 19 2023, 4:36 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
455

Out of an abundance of paranoia can you mask this with 0x3f. The original match was for a uimm which would allow 0-63. 0 would be a noop roate. I don't trust that the 0 won't show up since we are on target nodes and don't have DAG combines to clean it up if the 0 appears late. 64 - 0 would be 64 which would be an invalid value for the assembly. Masking 64 to 0 would prevent that from being a problem.

luke updated this revision to Diff 542457.Jul 20 2023, 5:39 AM

Mask off immediate

llvm/lib/Target/RISCV/RISCVInstrInfo.td
455

Good catch, done

This revision is now accepted and ready to land.Jul 20 2023, 10:10 PM
This revision was landed with ongoing or failed builds.Jul 21 2023, 2:23 AM
This revision was automatically updated to reflect the committed changes.