This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add vendor-defined XTheadBb (basic bit-manipulation) extension
ClosedPublic

Authored by philipp.tomsich on Feb 6 2023, 3:24 PM.

Details

Summary

The vendor-defined XTHeadBb (predating the standard Zbb extension)
extension adds some bit-manipulation extensions with somewhat similar
semantics as some of the Zbb 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 XTHeadBb 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

Depends on D143036

Diff Detail

Event Timeline

philipp.tomsich created this revision.Feb 6 2023, 3:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 3:24 PM
philipp.tomsich requested review of this revision.Feb 6 2023, 3:24 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 6 2023, 3:24 PM

Description says "XTHeadBs" instead of "XTHeadBb"

philipp.tomsich edited the summary of this revision. (Show Details)Feb 6 2023, 3:27 PM

Thanks for spotting this.
Fixed in our local git-tree as well, so it doesn't sneak back in during the merge.

I am blind to the difference between Ba, Bb, and Bs by now -- with Ba and Bs being too close to each other on the keyboard for comfort!.

  • reran clang-format (which of course caught something...)
  • rebase and update RISCVDisassembler.cc
craig.topper added inline comments.Feb 7 2023, 9:58 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
318

without these two lines to promote i32, I suppose we would get zext i32 to i64, ff1, addi? Is the sequence used for ctlzw better than that?

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

Why is TH_EXTU better than ANDI for uimm11 immediates?

315

The sra will very likely be turned into an srl if the next instruction doesn't use the upper bits. You probably need this pattern too

def : Pat<(binop_allwusers<srl> (bswap i64:$rs1), i64 32)), (TH_REVW i64:$rs1)>;
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
318

The sequences will have an identical critical-path (and require the same number of temporaries).
I read your comment as a recommendation to simplify the overall implementation (by removing the special case here and in the pattern-matching).

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:05 PM

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

craig.topper added inline comments.Feb 9 2023, 10:57 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
318

Yes that would be my recommendation if there's no reason to prefer one over the other.

  • updated for review comments
  • added release not entry
  • added missing test in attributes.ll
This revision is now accepted and ready to land.Feb 12 2023, 4:18 PM
This revision was landed with ongoing or failed builds.Feb 13 2023, 8:02 AM
This revision was automatically updated to reflect the committed changes.
inclyc added a subscriber: inclyc.Feb 13 2023, 8:47 AM
inclyc added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
315

It looks like this line of code will cause compilation warning.

[1677/1717] Building CXX object lib/Target/RISCV/CMakeFiles/LLVMRISCVCodeGen.dir/RISCVISelLowering.cpp.o
/tmp/llvm-project/llvm/lib/Target/RISCV/RISCVISelLowering.cpp:315:24: warning: braces around scalar initializer [-Wbraced-scalar-init]
    setOperationAction({ISD::CTLZ}, XLenVT, Legal);
                       ^~~~~~~~~~~
1 warning generated.
[1717/1717] Creating library symlink lib/libclang-cpp.so
[1542/1716] Building CXX object lib/Target/RISCV/CMakeFiles/LLVMRISCVCodeGen.dir/RISCVISelLowering.cpp.o
/tmp/llvm-project/llvm/lib/Target/RISCV/RISCVISelLowering.cpp:315:24: warning: braces around scalar initializer [-Wbraced-scalar-init]
    setOperationAction({ISD::CTLZ}, XLenVT, Legal);
                       ^~~~~~~~~~~
1 warning generated.
[1716/1716] Linking C executable bin/mlir-capi-execution-engine-test
320

And this one

inclyc added inline comments.Feb 13 2023, 8:49 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
320

Ah, this is my fault :(. There should be only 1 warning on line 315

philipp.tomsich marked 4 inline comments as done.Feb 13 2023, 8:53 AM
philipp.tomsich added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
315

Thanks. I'll push a NFC commit to fix this, as soon as my testrun completes.
Interesting that I don't see this warning on my build ... will dig to make sure that we are not missing anything in the future.