Page MenuHomePhabricator

[RISCV] Add support for the Zawrs extension
Needs ReviewPublic

Authored by palmer-dabbelt on Jun 20 2022, 5:37 PM.

Details

Summary

This extension contains a single instruction, wrs, which waits for the
current reservation set to be modified by a remote agent.

Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>


This is still some sort of very early draft and I'm not sure what the
rules are in LLVM land for merging that sort of code. It's a fast track
extension so it should be done soon, but it's been drifting around a bit
as part of that process so this might not be sane right now. I'm really
just doing this to check the box on LLVM support, which is just
assembler support due to the nature of the WRS instruction.

I also haven't gotten clang-format working yet, but I'm hoping something
in the CI will help with that.

Diff Detail

Event Timeline

palmer-dabbelt created this revision.Jun 20 2022, 5:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 5:37 PM
palmer-dabbelt requested review of this revision.Jun 20 2022, 5:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 20 2022, 5:37 PM

I've never touched LLVM before, so I have no idea if I'm even in the right place here...

eopXD added inline comments.Jun 20 2022, 5:40 PM
llvm/lib/Target/RISCV/RISCVInstrInfoZawrs.td
2

This doesn't match the filename.

eopXD added a comment.Jun 20 2022, 5:42 PM

I've never touched LLVM before, so I have no idea if I'm even in the right place here...

Yes, you are in the right place.
All LLVM patches go through the Phabricator.
A patch goes in when at least one person accepts it.

eopXD added a comment.EditedJun 20 2022, 5:47 PM
  • I think we will need some test coverage under llvm/test/CodeGen/RISCV/attributes.ll and llvm/test/MC/RISCV/attribute-arch.s if the extension is existing in the arch string.
  • Since macro was added, we need some test coverage under clang/test/Preprocessor/riscv-target-features.c
  • There are also some test cases for error handling under clang/test/Driver/riscv-arch.c, maybe add some there too?
  • Fix a whitespace issue
  • Fix the name of RISCVInstrInfoZawrs.td
  • Call the #define riscv_zawrs instead of riscv_wrs, to match the others
  • Add tests in clang/test/Driver/riscv-arch.c, clang/test/Preprocessor/riscv-target-features.c, llvm/test/CodeGen/RISCV/attributes.ll

The clang tests aren't running for me, presumably because clang isn't building locally. Also hoping to rely on CI for that one, but I'm not entirely sure.

eopXD added a comment.Jun 20 2022, 6:32 PM

Looks like the patch application failed, might need to rebase.

craig.topper added inline comments.Jun 20 2022, 6:54 PM
clang/test/Driver/riscv-arch.c
585

This doesn't match the RUN line

589

This doesn't match the RUN line

clang/test/Preprocessor/riscv-target-features.c
444

This CHECK directive doesn't match the RUN line

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

The convention is to use --check-prefix is there is only one prefix.

12

Append {{$}} to make the test stricter.

MaskRay added inline comments.Jun 20 2022, 9:04 PM
llvm/test/MC/RISCV/zawrs-invalid.s
4

[[@LINE]] is a deprecated FileCheck feature.

Use [[#@LINE]]

Note: you can place negative tests with the positive test file. Search .ifndef err.

kito-cheng added inline comments.Jun 20 2022, 11:45 PM
clang/lib/Basic/Targets/RISCV.cpp
195

You don't need this line, that would be defined automatically once you've add zawrs in RISCVISAInfo.cpp, see the loop in the earlier in this function, it's done by there:

for (auto &Extension : ISAInfo->getExtensions()) {
  auto ExtName = Extension.first;
  auto ExtInfo = Extension.second;
  unsigned Version =
      (ExtInfo.MajorVersion * 1000000) + (ExtInfo.MinorVersion * 1000);

  Builder.defineMacro(Twine("__riscv_", ExtName), Twine(Version));
}
asb added a comment.Jun 21 2022, 1:30 AM

This is still some sort of very early draft and I'm not sure what the rules are in LLVM land for merging that sort of code. It's a fast track extension so it should be done soon

The policy we ended up with is that support for not-yet-ratified extensions can be merged, but they're made to stick out like a sore thumb by calling the feature -experimental-foo and requiring -menable-experimental-extensions in clang. Given the simplicity of zawrs there's substantially less scope for drastic changes, but I think conforming to that is probably the best patch forwards. You should be able to see an example by looking at one of the stalled bitmanip extensions, e.g. experimental-zbp.