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

[Philipp Tomsich via Phabricator via llvm-commits]

philipp.tomsich 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();
----------------
mtsamis wrote:
> 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
The problem really is the Specification on this one:
   th.ldd rd1,rd2,(rs1),#imm2,4
appears to have 4 as an operand (even though it is the only permissible value and does not get encoded explicitly).

However, looking at it from another angle the ",4" is part of the operation mnemonic (or an %_ term when expressing the pattern in GNU notation):
   th.ldd %1,%2,(%0),%3,4
Given that the 4 is not encoded, I am leaning with Craig at the moment and consider it as part of the mnemonic (i.e., a split off fragment of 'th.ldd'+',4').

I would much rather have seen this specified as
    th.ldd rd1,rd2,#offset(rs1)
where offset would have a constraint to have its lower 4 bits cleared.

This would be a much more meaningful assembler syntax.
Can we have this as a pseudo (that also becomes the preferred disassembly)?
I'll volunteer to send out a PR against the T-Head specification to add that additional assembler-pseudo under the th.ldd definition…


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144002