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

Craig Topper via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 20 16:45:15 PST 2023


craig.topper added a comment.

LGTM with the noted tab characters fixed. Please double check there aren't any others.



================
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:
> > 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.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:124
+            (outs GPR:$rd, GPR:$rs2),
+	    (ins GPR:$rs1, uimm2:$uimm2, uimm7:$const3or4),
+  opcodestr, "$rd, $rs2, (${rs1}), $uimm2, $const3or4"> {
----------------
I think there's a tab character here instead of spaces


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:137
+            (outs),
+	    (ins GPR:$rd, GPR:$rs2, GPR:$rs1, uimm2:$uimm2, uimm7:$const3or4),
+  opcodestr, "$rd, $rs2, (${rs1}), $uimm2, $const3or4"> {
----------------
I think there's a tab character here instead of spaces


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:22
+
+def TH_TLWUD : SDNode<"RISCVISD::TH_LWUD", SDT_TDBLD,
+  [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
----------------
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'


================
Comment at: llvm/test/CodeGen/RISCV/xtheadmempair.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -mattr=+xtheadmempair -verify-machineinstrs < %s \
----------------
mtsamis wrote:
> craig.topper wrote:
> > Can we test i64 load/store on rv32 and i128 load on rv64? I assume we use these instructions to handle the split.
> Ok, I have added four new test functions: ld64/ld128/sd64/sd128.
> All tested for both rv32/rv64, I believe that should be fine.
> The expected instructions are emmited for the loads/stores.
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144002



More information about the llvm-commits mailing list