This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add vendor-defined XTHeadBs (single-bit) extension
ClosedPublic

Authored by philipp.tomsich on Jan 31 2023, 5:10 PM.

Details

Summary

The vendor-defined XTHeadBs (predating the standard Zbs extension)
extension adds a bit-test instruction (th.tst) with similar semantics
as bexti from Zbs. 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 XTHeadBs 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, 5:10 PM
philipp.tomsich requested review of this revision.Jan 31 2023, 5:10 PM

The current (as of this commit) public documentation for XTHeadBa is

Copy/paste error

craig.topper added inline comments.Jan 31 2023, 5:48 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
799

Update comment to include th.tst

930

Update comment

  • fix typo 'Ba -> Bs' in commit message
  • update comments in ISelDAGToDAG to refer to bexti _and th.tst_
philipp.tomsich edited the summary of this revision. (Show Details)Feb 1 2023, 4:01 AM
llvm/test/CodeGen/RISCV/rv32xtheadbs.ll
8

Rename these tests from bexti to th_tst?

  • updated the test cases to have function names matching the instructions
philipp.tomsich marked an inline comment as done.Feb 2 2023, 3:57 AM

Any plan to support RISCVTargetLowering::isMaskAndCmp0FoldingBeneficial, RISCVTargetLowering::hasBitTest, performTRUNCATECombine or performANDCombine?

Any plan to support RISCVTargetLowering::isMaskAndCmp0FoldingBeneficial, RISCVTargetLowering::hasBitTest, performTRUNCATECombine or performANDCombine?

Yes, this is planned in a follow-up optimization ... if that is ok?
The plan is to get the basic support (such as in these initial patches) in and then add more optimizations as we run SPEC against the full set of extensions.

For XTHeadBb (which is the next one from the series that will go to Phabricator), there is some DAGToDAG work to support bit-extract instructions (i.e. similar to how AArch64 handles the formation of UBFM and SBFM nodes).
This will also be split off into a separate patch.

Ok to defer? Or would you rather have this go in with the initial enablement?

Thinking some more about this, I'll just add the associated changes onto the initial patch for th.tst.
I have to double-check hasBitTest(), as XTHeadBs only has an immediate-form of the instruction (i.e., bexti only, but no bext).

  • wire up to existing optimisations in RISCVISelLowering.cpp
  • rewrite RISCVTargetLowering::hasBitTest() to differentiate between immediate and non-immediate cases
reames added a subscriber: reames.Feb 2 2023, 12:04 PM

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

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1243 ↗(On Diff #494387)

Style wise, a series of early returns here would be easy to follow.

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

Missing decoder namerspace

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

In this diff, you have coverage for both 32 and 64, but is this extension actually defined for RV32? The previous diff (THeadBa) was only tested on RV64. Why are these two handled differently here?

philipp.tomsich marked 2 inline comments as done.
  • added decoder namespaces
llvm/test/CodeGen/RISCV/rv32xtheadbs.ll
2

In this diff, you have coverage for both 32 and 64, but is this extension actually defined for RV32? The previous diff (THeadBa) was only tested on RV64. Why are these two handled differently here?

Test cases to THeadBa have been added in the latest revisison.

philipp.tomsich marked an inline comment as done.Feb 3 2023, 1:10 PM
philipp.tomsich marked an inline comment as done.
philipp.tomsich marked an inline comment as done.
  • add missing handling of the XTHeadBs decoder namespace in RISCV disassembler
  • include llvm/test/CodeGen/RISCV/bittest.ll into the test-coverage for XTHeadBs
craig.topper added inline comments.Feb 6 2023, 3:38 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
801

Maybe put into a variable like bool HasBitTest = Subtarget->hasStdExtZbs() || Subtarget->hasVendorXTHeadBs() then use it here and on line 969 for the Skip?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1252 ↗(On Diff #495029)

We don't use NULL in llvm. Use nullptr.

8709 ↗(On Diff #495029)

I think this code was for variable shifts is it relevant for this extension?

8957 ↗(On Diff #495029)

I think this code was for variable shifts is it relevant for this extension?

  • rerun clang-format
  • update the 'Depends on' to (hopefully) fix automatic the CI for the stack
philipp.tomsich marked 4 inline comments as done.
  • address Craig's review comments (style, NULL->nullptr and 2 variable shift paths)

Why does this patch depend on the MULCombine patch?

craig.topper added inline comments.Feb 7 2023, 9:30 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
931–932

These two lines can be merged

Skip |= HasBitTest && Leading == XLen - 1

This revision was not accepted when it landed; it landed in state Needs Review.Feb 7 2023, 10:57 PM
This revision was automatically updated to reflect the committed changes.
philipp.tomsich reopened this revision.Feb 7 2023, 11:04 PM

Reopening as this was accidentially pushed and reverted when doing 'arc patch on D143534'.

  • merged HasBitTest tests
philipp.tomsich marked an inline comment as done.Feb 12 2023, 2:59 PM
This revision is now accepted and ready to land.Feb 12 2023, 3:26 PM
This revision was landed with ongoing or failed builds.Feb 13 2023, 7:28 AM
This revision was automatically updated to reflect the committed changes.