This is an archive of the discontinued LLVM Phabricator instance.

[RISC-V] Add proposed mapping for Ztso
ClosedPublic

Authored by patrick-rivos on Jul 17 2023, 2:27 PM.

Details

Summary

Currently LLVM emits Ztso code for fences, loads, and stores (behind an experimental flag) [1]. This patch updates the mapping and implements support for LR/SC and AMO ops. This updated mapping is compatible with the RVWMO ABI present in the psABI. Additional context can be found in the psABI pull request [2].

[1] https://reviews.llvm.org/D143076
[2] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/391

Diff Detail

Event Timeline

patrick-rivos created this revision.Jul 17 2023, 2:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 2:27 PM
patrick-rivos requested review of this revision.Jul 17 2023, 2:27 PM
asb added a comment.Jul 19 2023, 6:25 AM

From a first pass through, this appears to match the proposal. I've left one style comment that should be addressed.

I'm not convinced by the new helper in RISCVInstrInfo - might it be cleaner to just pass the RISCVSubtarget to the helpers? I think conventionally we'd put the subtarget argument last for this kind of helper

llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp
165

LLVM style is to not use an else after return. https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Same applies elsewhere in this patch

Meta comment - mostly just chiming is because I'm the person who did the prior implementation, and I want it to be clear I support the direction here.

Given what looks to be a defacto change in the WMO memory model lowering which is going to be widely deployed, we should change the TSO mapping to be compatible with the planned mapping. The goal of the original change was to allow WMO and TSO objects to be compatible; this change continues in the same spirit, it's just that the assumed WMO lowering is changing.

This case - which we did not know about at the time of the original patch - is exactly why we left ourselves the flexibility of only supporting this experimentally. We absolutely should not, under any circumstances, promote this out of experimental until the psABI changes for WMO and TSO have finalized.

Revised to address @asb review comments.

I had to unprotect the subtarget attribute in order to pass it to the helper functions. If there's a better way to pass the subtarget please let me know!

patrick-rivos marked an inline comment as done.Jul 19 2023, 6:28 PM
asb added a comment.Jul 20 2023, 6:56 AM

@patrick-rivos: I've just posted D155840 to introduce a field for RISCVSubtarget in those pseudo expansion passes. I think that would probably be a good approach here, but you might want to wait for any feedback on that review before changing again in case others disagree with me!

LGTM once we resolve that (I'm expecting a quick review on that linked patch - if it's slow for some reason let's loop back and make sure this patch isn't blocked on it unreasonably). Thanks for such a rapid update, and of course for updating us to the most recent proposed mappings.

asb added a comment.Jul 21 2023, 8:52 AM

Ok, D155840 was reviewed and committed so I think following that same approach here (and leaving the STI field of RISCVInstrInfo as protected) probably makes sense. Then this patch LGTM. Thanks for your patience on these minor implementation tweaks.

Revised to keep the STI field of RISCVInstrInfo protected and get STI in runOnMachineFunction (Same approach as D155840).
Also removed a stray else I had missed on the earlier revision.

asb accepted this revision.Jul 21 2023, 11:48 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 21 2023, 11:48 AM
asb added a comment.EditedAug 9 2023, 3:35 AM

The psABI Ztso proposal was merged today: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/391

Brilliant. My LGTM still stands so I think this is good to merge once it's rebased (I think you basically just need to regenerate the test changes with update_llc_test_checks.py). Do you have commit access?

Rebased to main and regenerated changes to the testsuite.

Do you have commit access?

AFAIK no I don't have commit access.

This revision was landed with ongoing or failed builds.Aug 10 2023, 7:59 AM
This revision was automatically updated to reflect the committed changes.
asb added a comment.Aug 10 2023, 8:01 AM

I've committed for you. Thanks for working on this!

Great, thank you!

llvm/test/CodeGen/RISCV/atomic-load-store.ll