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

[Manolis Tsamis via Phabricator via llvm-commits]

mtsamis marked 6 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:
> > > 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
> But if `Found` is true in the first call, I think the second call to `hasPredecessorHelper` won't happen because the first call will return `true` so we won't continue past the `||`. So the worklist is empty in the case that the second call to `hasPredecessorHelper` happens.
That's true.
Then given that worklist is empty, the second call always returns false except for when LSnode2 has been visited in the first call?

```
if (Visited.count(N))
      return true;
```


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:22
+
+def TH_TLWUD : SDNode<"RISCVISD::TH_LWUD", SDT_TDBLD,
+  [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
----------------
craig.topper wrote:
> mtsamis wrote:
> > 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.
> I was referring to the second 'T' in TH_TLWD. Not the 'T' that was part of 'TH'
Ok makes sense, I misread that. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144002