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

[Manolis Tsamis via Phabricator via llvm-commits]

mtsamis marked 3 inline comments as done.
mtsamis added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9731
+
+  if (SDNode::hasPredecessorHelper(LSNode1, Visited, Worklist) ||
+      SDNode::hasPredecessorHelper(LSNode2, Visited, Worklist))
----------------
craig.topper wrote:
> 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.
Indeed, that's true. After looking again at the code though there is a 

```
if (Op == N)
  Found = true;
...
if (Found)
  break;
```
which means that the Worklist may still be empty at the end.
Given that the worklist is initially {LSNode1, LSNode2} and N is LSNode1 at the first call and LSNode2 at the second, I think the worklist potentially contains LSNode1 and/or LSNode2 at the second call too


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:22
+
+def TH_TLWUD : SDNode<"RISCVISD::TH_LWUD", SDT_TDBLD,
+  [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
----------------
craig.topper wrote:
> 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?
Done, but I've kept the th prefix because otherwise there could be a potential name collision with future changes?
I also renamed SDT_TDBLD and SDT_TDBST to SDT_LoadPair and SDT_StorePair, which I think are more readable.


================
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"> {
----------------
craig.topper wrote:
> 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.
Done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144002