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

[Manolis Tsamis via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2672
+    // The last operand must be constant 3 or 4 depending on the data width.
+    if (IsWordOp && Inst.getOperand(4).getImm() != 3) {
+      SMLoc Loc = Operands.back()->getStartLoc();
----------------
craig.topper wrote:
> craig.topper wrote:
> > Why does this 3rd operand exist at all?
> Oops, I meant fifth operand.
Although I also don't see why this required, I implemented it as per the specification


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9744
+  MachineMemOperand *NewMMO = MF.getMachineMemOperand(
+      MMO, MMO->getPointerInfo(), MemVT == MVT::i32 ? 8 : 16);
+
----------------
craig.topper wrote:
> Since the offset allows for a gap in the memory locations the size isn't 8/16. It needs to be UnknownSize or large enough to cover the gap.
The offset does not allow for a gap, all loads stores are from adjacent memory locations.
The offset only affects the base pointer from which the load/store happens.

E.g. per the specification:
  addr := rs1 + (zero_extend(imm2) << 4)
  tmp1 := mem[addr+7:addr]
  tmp2 := mem[addr+15:addr+8]

Would that make the 8/16 correct then?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9847
+        // Check for adjucent i64 values and a 2-bit index.
+        if ((Offset1 + 8 != Offset2) || !isShiftedUInt<2, 4>(Offset1))
+          continue;
----------------
craig.topper wrote:
> How do we know MemVT is i64 here?
You're correct that this is not known; I've added a check. Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144002