[PATCH] D64125: [RISCV] Support @llvm.readcyclecounter() Intrinsic

[Alex Bradbury via Phabricator via llvm-commits]

asb requested changes to this revision.
asb added a comment.
This revision now requires changes to proceed.

Thanks Sam - left a number of comments in-line.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:160
   }
+  case RISCVISD::READ_CYCLE_WIDE:
+    ReplaceNode(Node, CurDAG->getMachineNode(RISCV::ReadCycleWide, DL, MVT::i32,
----------------
Couldn't hurt to add an assert here that that `!Subtarget.is64Bit()`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:183
 
+  // READCYCLECOUNTER will use RDCYCLE[H]
+  setOperationAction(ISD::READCYCLECOUNTER, MVT::i64,
----------------
Nit: should have full stop at end of comment


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:843
     llvm_unreachable("Don't know how to custom type legalize this operation!");
+  case ISD::READCYCLECOUNTER: {
+    SDVTList VTs = DAG.getVTList(MVT::i32, MVT::i32, MVT::Other);
----------------
Should add an assert that `!Subtarget.is64Bit()`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:845
+    SDVTList VTs = DAG.getVTList(MVT::i32, MVT::i32, MVT::Other);
+    SDValue RTB =
+        DAG.getNode(RISCVISD::READ_CYCLE_WIDE, DL, VTs, N->getOperand(0));
----------------
RTB isn't really a meangful name. `RCW`?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1070
+
+  MachineBasicBlock *readMBB = MF.CreateMachineBasicBlock(LLVM_BB);
+  MachineBasicBlock *sinkMBB = MF.CreateMachineBasicBlock(LLVM_BB);
----------------
Current style would be ReadMBB, though I'd be tempted to call this `LoopMBB`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1071
+  MachineBasicBlock *readMBB = MF.CreateMachineBasicBlock(LLVM_BB);
+  MachineBasicBlock *sinkMBB = MF.CreateMachineBasicBlock(LLVM_BB);
+  DebugLoc dl = MI.getDebugLoc();
----------------
Current style would be SinkMBB though I'd be temped to call this `DoneMBB`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1072
+  MachineBasicBlock *sinkMBB = MF.CreateMachineBasicBlock(LLVM_BB);
+  DebugLoc dl = MI.getDebugLoc();
+  MF.insert(It, readMBB);
----------------
Should be DL


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1082
+  BB->addSuccessor(readMBB);
+  BB = readMBB;
+
----------------
When my select custom insertion was being reviewed, the consensus was that reassigning the `BB` pointer made the logic needlessly hard to follow for little benefit. I suggest just referring to `LoopMBB` directly instead.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1060
+/// readcyclecounter
+// On RV64, we can directly use "rdcycle"
+let Predicates = [IsRV64] in
----------------
Given we you have to list this as CSRRS, I'd phrase this as "we can directly access the 64-bit 'cycle' CSR."


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1063
+def : Pat<(readcyclecounter), (CSRRS 0xC00, X0)>;
+// On RV32, ReadCycleWide is inserted and expanded to the suggested rdcycle[h] loop.
+let Predicates = [IsRV32], usesCustomInserter = 1, hasSideEffects = 0,
----------------
"to the suggested loop reading the 'cycle' and 'cycleh' CSRs"?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1066
+mayLoad = 0, mayStore = 0, hasNoSchedulingInfo = 1 in
+def ReadCycleWide : Pseudo<(outs GPR:$lo, GPR:$hi), (ins), [], "#ReadCycleWide", "($hi, $lo)">;
+
----------------
Given opcodestr+argstr should't ever be emitted, I think it makes sense to just remove those strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64125