[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:1138
+                               const RISCVSubtarget &Subtarget) {
+  if (Op.getSimpleValueType() == Subtarget.getXLenVT()) {
+    auto LHS = matchRISCVBitmanipPat(Op.getOperand(0), IsW);
----------------
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.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1145
+          IsW ? RISCVISD::GREVIW : RISCVISD::GREVI, DL, Op.getValueType(),
+          LHS->Op, DAG.getConstant(LHS->ShAmt, DL, Subtarget.getXLenVT()));
+    }
----------------
craig.topper wrote:
> Since this is expected to be a constant, can we use getTargetConstant here and use timm instead of imm in the isel patterns?
Sounds good to me.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1282
+                                    /*IsW*/ false, DCI.DAG, Subtarget))
+      return DCI.CombineTo(N, GREV);
+    if (auto GORC = combineORToGORC(SDValue(N, 0),
----------------
craig.topper wrote:
> Can this just be "return GREV;"?
Yes, I think so. Thanks.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2814
     return "RISCVISD::READ_CYCLE_WIDE";
+  case RISCVISD::GREVI:
+    return "RISCVISD::GREVI";
----------------
craig.topper wrote:
> General question for the RISCV backend, would it make sense to use a macro to generate these cases like X86 and AMDGPU do
> 
> #define NODE_NAME_CASE(NODE) case RISCVISD::NODE: return "RISCVISD::" #NODE;
> 
> This would eliminate having to mention the node name twice. On X86 I found a couple typos in the strings so having them automatically created can remove human error there.
I'm glad you brought it up as I'm all in favour of that approach. I can do that as a separate patch to gather feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91259