This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add vendor-defined XTHeadBa (address-generation) extension
ClosedPublic

Authored by philipp.tomsich on Jan 31 2023, 4:17 PM.

Details

Summary

The vendor-defined XTHeadBa (predating the standard Zba extension)
extension adds an address-generation instruction (th.addsl) with
similar semantics as sh[123]add from Zba. 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 XTHeadBa is
available from:

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=8254c3d2c94ae5458095ea6c25446ba89134b9da

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 4:17 PM
philipp.tomsich requested review of this revision.Jan 31 2023, 4:17 PM

Update docs/ReleaseNotes.rst too

llvm/test/CodeGen/RISCV/rv64xtheadba.ll
63

sh2add -> th.addsl

  • Update llvm/docs/ReleaseNotes.rst
  • Fix comment in testcase: sh2add -> th.addsl
philipp.tomsich marked an inline comment as done.Jan 31 2023, 5:12 PM
  • fixup .ll tests to not reference an unused check-label
llvm/test/CodeGen/RISCV/rv64xtheadba.ll
8

Rename these tests from sh[1,2,3]add to th_addsl_[1,2,3]?

67

And this one too.

  • updated the test cases to have function names matching the instructions
philipp.tomsich marked 2 inline comments as done.Feb 2 2023, 3:58 AM
reames added a subscriber: reames.Feb 2 2023, 11:46 AM

This generally looks reasonable to me, and I support adding this vendor extension once normal code review is complete.

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

Stray whitespace.

46

It looks like you're missing the decoder namerspace here.

llvm/test/MC/RISCV/rv64xtheadba-valid.s
10 ↗(On Diff #494246)

Is 0 a valid encoding? If so, can you add a test covering that?

p.s. The choice to use uimm2 seems slightly odd here since it wastes one value for the shift. Was the 0 encoding possibly used for something else? If it was, maybe we need a different immediate type or an assert somewhere?

llvm/test/MC/RISCV/rv64xtheadba-valid.s
10 ↗(On Diff #494246)

Yes, 0 is a valid encoding.
Yes, it wastes encoding space and is non-sensical and should normally not be generated.

We'll add the test case. We already have the equivalent in the binutils tests (see https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/testsuite/gas/riscv/x-thead-ba.s;h=5081c06e6aa85ec4ec0882780be30b25a80e11b1;hb=8254c3d2c94ae5458095ea6c25446ba89134b9da)

reames added inline comments.Feb 2 2023, 12:32 PM
llvm/test/MC/RISCV/rv64xtheadba-valid.s
10 ↗(On Diff #494246)

Great, that's the ease answer implementation wise. :)

  • adds rv32 test cases
  • adds test for th.addsl rd,rs1,rs2,0
  • adds decoder namespace
philipp.tomsich marked 3 inline comments as done.Feb 3 2023, 1:07 PM
philipp.tomsich marked an inline comment as done.

Why didn't the DecoderNamespace require changes to RISCVDisassembler::getInstruction to lookup in the correct table?

craig.topper added a comment.EditedFeb 3 2023, 6:40 PM

Why didn't the DecoderNamespace require changes to RISCVDisassembler::getInstruction to lookup in the correct table?

I applied this patch locally and the tests fail. So I guess it does changes?

  • Add missing handling of the XTHeadBa decoder namespace in the RISCV Disassembler

LGTM

llvm/test/MC/RISCV/rv32xtheadba-valid.s
1 ↗(On Diff #495023)

I think this "Bitmanip base" was copied from Zbb? Should we adjust it to include T-Head?

llvm/test/MC/RISCV/rv64xtheadba-invalid.s
1 ↗(On Diff #495023)

Can we merge this test with rv32xtheadba-invalid.s?

llvm/test/MC/RISCV/rv64xtheadba-valid.s
1 ↗(On Diff #495023)

Can we merge this with rv32xtheadba-valid.s?

  • make clang-format happy
craig.topper added inline comments.Feb 6 2023, 9:26 PM
llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
479

I update this function so we don't have to keep repeating Size = 4. Please rebase.

philipp.tomsich marked 3 inline comments as done.
  • rebased and updated RISCVDisassembler.cpp
  • merged RV32 and RV64 assembler tests into a single file
philipp.tomsich added inline comments.Feb 7 2023, 5:10 AM
llvm/test/MC/RISCV/rv32xtheadba-valid.s
1 ↗(On Diff #495023)

This can't be merged, as the mnemonics are different.
Updated the comment.

craig.topper accepted this revision.Feb 7 2023, 9:26 AM

LGTM other than test comment.

llvm/test/MC/RISCV/XTHeadBa-valid.s
2

"base" is wrong here, isn't it? This is the addressing extension. Is it wrong in the Zba test too?

This revision is now accepted and ready to land.Feb 7 2023, 9:26 AM
llvm/test/MC/RISCV/XTHeadBa-valid.s
2

Yes, it is wrong in the Zba tests: I'll send up NFC patches for those.

That said: I admire your attention to detail.

philipp.tomsich marked an inline comment as done.
  • fixes comment in XTHeadBa-valid.s ("bitmanip base" -> XTHeadBa)