This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add vendor-defined XTheadMemPair (two-GPR Memory Operations) extension
ClosedPublic

Authored by mtsamis on Feb 14 2023, 6:00 AM.

Details

Summary

The vendor-defined XTHeadMemPair (no comparable standard extension exists
at the time of writing) extension adds two-GPR load/store pair instructions.

It is supported by the C9xx cores (e.g., found in the wild in the
Allwinner D1) by Alibaba T-Head.

The current (as of this commit) public documentation for this
extension is available at:

https://github.com/T-head-Semi/thead-extension-spec/releases/download/2.2.2/xthead-2023-01-30-2.2.2.pdf

Support for these instructions has already landed in GNU Binutils:

https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=6e17ae625570ff8f3c12c8765b8d45d4db8694bd

Depends on D143847

Diff Detail

Event Timeline

mtsamis created this revision.Feb 14 2023, 6:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2023, 6:00 AM
mtsamis requested review of this revision.Feb 14 2023, 6:00 AM
craig.topper added inline comments.Feb 14 2023, 10:17 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2678

Does this restrict apply for TH_SDD/TH_SWD? I don't see it in the spec.

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
452

Drop curly braces

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9680

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.

9739

Drop curly braces

9778

adjacent*

9782

adjacent*

9783

How do we know MemVT is i64 here?

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2678

Given that QEMU had this constraint, but it was missing in the spec, I checked on an Allwinner D1.
Turns out that LDD works with with $rs1 == $rs2 and with $rs1 == $rs2 == $rd.

craig.topper added inline comments.Feb 14 2023, 2:36 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2678

Did you mean SDD instead of LDD?

craig.topper added inline comments.Feb 14 2023, 2:37 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

Why does this 3rd operand exist at all?

2685

Oops, I meant fifth operand.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2678

Yes, the store instructions (SDD and SWD) work.

mtsamis updated this revision to Diff 497626.Feb 15 2023, 3:45 AM
  • Do not restrict source and destination registers for TH_SDD/TH_SWD
  • Add missing check for MemVT == i64
  • Fix typos and remove unwanted braces
mtsamis marked 9 inline comments as done.Feb 15 2023, 4:27 AM
mtsamis added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

Although I also don't see why this required, I implemented it as per the specification

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9680

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?

9783

You're correct that this is not known; I've added a check. Thanks

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

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…

mtsamis marked an inline comment as done.Feb 15 2023, 7:55 AM
mtsamis added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

From what I understand, even if we have an additional assembler-pseudo for a nicer syntax we would still have to support the current one from the specification. In that case, adding the 'nice' syntax won't make the implementation better because we'll have to support the other one as well. The "4" is indeed not encoded and could be part of the assembler string as + ", 4" but from what I understood when implementing it, this doesn't work great with the current infrastructure. You can also check this discussion about implicit operands from which I got ideas how to properly implent them: mc-layer-parsing-disassembling-implicit-operands. The constant assembler string makes the assembler not able to properly parse the instruction and also makes it hard to get proper error messages as in this implementation.

Tl;dr My personal conclusion is that if we have to support the current syntax then we don't benefit from creating an alternative nicer syntax.

jrtc27 added inline comments.Feb 15 2023, 8:01 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

lw rd, rs1, imm is the syntax in the unprivileged spec, but lw rd, imm(rs1) is the canonical assembly format. Ditto for jalr. You could just view it the same way.

mtsamis added inline comments.Feb 15 2023, 8:05 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

But the mempair instruction are of the form th.ldd rd1, rd2, (rs1), imm2, 4 where there is both imm but also a trailing 3 or 4 which has no function (as discussed above) and is not included in the instruction encoding. But it still has to be there as per the specification and the assembler/disassembler must work with it.

So I don't think this is the same as lw?

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

I agree with Jessica in principle, but we have a released GNU Binutils in the wild that implements the "weird" assembler syntax. So we should not have a hard break for backwards compatibility (i.e., will need coexistence).

jrtc27 added inline comments.Feb 15 2023, 8:10 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

I wasn't suggesting ditching the weird syntax; lw rd, rs1, imm is legal assembly and works in both assemblers today. Just saying we can (a) have multiple syntaxes supported (b) change the canonical one.

craig.topper added inline comments.Feb 15 2023, 8:43 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9680

Yes. You're right. Sorry about that.

mtsamis marked 2 inline comments as done.Feb 15 2023, 11:13 AM
mtsamis added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9680

No problem, marking as done then

mtsamis marked an inline comment as done.Feb 15 2023, 11:24 AM
mtsamis added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

Jessica, I think I now better understand what you've meant and it sounds like a great way to do this.

Then, would it be possible for the alias to be "th.ldd $rd1, $rd2, ($rs1), $offset, 4" where the "4" part is discarded?
I'm not familiar enough with the infrastructure to know if this will work fine with the assembler/disassembler, but I can try it out.

Please correct me if I'm wrong or understood your proposal incorrectly.

craig.topper added inline comments.Feb 15 2023, 11:39 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

I don't think InstAlias or the Instruction AsmString support a specific constant hardcoded into the string. So it would still need to be parsed as an immediate.

mtsamis added inline comments.Feb 15 2023, 11:50 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

Ok, I see. Then I can add the alias for the alternative syntax if we'd like to, without that affecting the current implementation.

craig.topper added inline comments.Feb 15 2023, 4:49 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

Is the 3 or 4 indicating the amount the immediate is shifted by? Instead of only accepting immediates that are divisible 8 or 16. The latter is what C.LW does for example.

mtsamis added inline comments.Feb 16 2023, 12:39 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

Is the 3 or 4 indicating the amount the immediate is shifted by?

Yes, that looks to be the case.

Instead of only accepting immediates that are divisible 8 or 16. The latter is what C.LW does for example.

I looked up the format of C.LW and more conveniently the (shifted) constant is a single argument of the instruction with the restriction you mentioned. In MemPair the two significant bits of the constant and the shift amount are separate arguments but the shift is reduntant anyway.

craig.topper added inline comments.Feb 16 2023, 12:45 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

I don't know if this instruction was defined before C.LW or just ignored the precedent.

But I guess it doesn't matter now. We have to implement the spec.

mtsamis added inline comments.Feb 16 2023, 12:50 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

Yes, I agree with that; the format for these instructions is a bit unfortunate.

So, of all things that were discussed here, is there some desired change I should do on the current implementation?

craig.topper added inline comments.Feb 16 2023, 11:20 AM
llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
453

Inst.addOperand(MCOperand::createImm(IsWordOp ? 3 : 4));

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9774

contraints -> constraints

llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td
14

What is "C" here?

llvm/test/CodeGen/RISCV/xtheadmempair.ll
2

Can we test i64 load/store on rv32 and i128 load on rv64? I assume we use these instructions to handle the split.

craig.topper added inline comments.Feb 16 2023, 11:27 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

I don't think we can change the canonical syntax without binutils also supporting or we will break -fno-integrated-as.

I'm ok with just accepting the syntax binutils supports.

craig.topper added inline comments.Feb 16 2023, 11:29 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9667

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?

mtsamis updated this revision to Diff 498312.Feb 17 2023, 3:54 AM

Fix typos and add additional tests.

mtsamis marked 14 inline comments as done.Feb 17 2023, 4:07 AM
mtsamis added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2685

Ok, marking this as done then.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9667

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?

llvm/test/CodeGen/RISCV/xtheadmempair.ll
2

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.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 17 2023, 10:45 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
philipp.tomsich reopened this revision.Feb 17 2023, 10:47 AM

Accidentially pushed and reverted.

craig.topper added inline comments.Feb 17 2023, 3:26 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9667

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.

llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td
22

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?

123

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.

135

Same here

mtsamis updated this revision to Diff 498761.Feb 20 2023, 1:28 AM
mtsamis marked 2 inline comments as done.

Correctly push the latest changes.

mtsamis updated this revision to Diff 498793.Feb 20 2023, 4:09 AM

Change names of MemPair nodes and code alignment

mtsamis marked 3 inline comments as done.Feb 20 2023, 4:17 AM
mtsamis added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9667

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

llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td
22

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.

123

Done

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

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9667

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.

llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td
22

I was referring to the second 'T' in TH_TLWD. Not the 'T' that was part of 'TH'

124

I think there's a tab character here instead of spaces

137

I think there's a tab character here instead of spaces

llvm/test/CodeGen/RISCV/xtheadmempair.ll
2

Thanks!

craig.topper accepted this revision.Feb 20 2023, 4:45 PM
This revision is now accepted and ready to land.Feb 20 2023, 4:45 PM
mtsamis updated this revision to Diff 499048.Feb 20 2023, 11:35 PM
mtsamis marked 2 inline comments as done.

Replace tabs with spaces and fix alignment

mtsamis marked 6 inline comments as done.Feb 20 2023, 11:42 PM
mtsamis added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9667

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;
llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td
22

Ok makes sense, I misread that.

This revision was landed with ongoing or failed builds.Feb 21 2023, 3:22 AM
This revision was automatically updated to reflect the committed changes.