Page MenuHomePhabricator

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

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



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:

Support for these instructions has already landed in GNU Binutils:;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


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
pcwang-thead added inline comments.Feb 1 2023, 10:53 PM

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


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.


Stray whitespace.


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

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?

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;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
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


1 ↗(On Diff #495023)

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

1 ↗(On Diff #495023)

Can we merge this test with rv32xtheadba-invalid.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

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


"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

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)