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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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);
----------------
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.


================
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()));
+    }
----------------
Since this is expected to be a constant, can we use getTargetConstant here and use timm instead of imm in the isel patterns?


================
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),
----------------
Can this just be "return GREV;"?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2814
     return "RISCVISD::READ_CYCLE_WIDE";
+  case RISCVISD::GREVI:
+    return "RISCVISD::GREVI";
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91259