[PATCH] D144002: [RISCV] Add vendor-defined XTheadMemPair (two-GPR Memory Operations) extension

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9731
+
+  if (SDNode::hasPredecessorHelper(LSNode1, Visited, Worklist) ||
+      SDNode::hasPredecessorHelper(LSNode2, Visited, Worklist))
----------------
mtsamis wrote:
> craig.topper wrote:
> > Just to make sure I understand. The first call processes the worklist. The second call will have an empty worklist so the only thing the second call does is check whether LSNode2 appears in the visited set from the first call?
> This code is taken from elsewhere in LLVM and as per the comments (e.g. in DAGCombiner.cpp::getPostIndexedLoadStoreOp) it checks if the two nodes are independent of each other.
> Otherwise the merged load/store may be incorrect. Also I believe that the Worklist is not always empty in the second call; hasPredecessorHelper addes back some nodes to the worklist after it has finished. 
> 
> Since this pattern is followed in other places, is this code fine to use in the same way here?
I think the DeferredNodes that are pushed back to the worklist only exist if `TopologicalPrune` is true which it isn't here.

The code is fine to use here. I just wanted to make sure I understood how it worked.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:22
+
+def TH_TLWUD : SDNode<"RISCVISD::TH_LWUD", SDT_TDBLD,
+  [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
----------------
Can we name these with lower case letters. I think that's consistent with the usual style. That should also remove the need for the extra T prefix?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:123
+  : RVInstR<!shl(funct5, 2), 0b100, OPC_CUSTOM_0,
+  (outs GPR:$rd, GPR:$rs2), (ins GPR:$rs1, uimm2:$uimm2, uimm7:$const3or4),
+  opcodestr, "$rd, $rs2, (${rs1}), $uimm2, $const3or4"> {
----------------
Can you indent these to line up with `!shl` on the previous line. Probably need a line break for 80 columns. But it will provide a better visual separation from the `let` statements in the body.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:135
+  : RVInstR<!shl(funct5, 2), 0b101, OPC_CUSTOM_0,
+  (outs), (ins GPR:$rd, GPR:$rs2, GPR:$rs1, uimm2:$uimm2, uimm7:$const3or4),
+  opcodestr, "$rd, $rs2, (${rs1}), $uimm2, $const3or4"> {
----------------
Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144002