This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Implement a proposed mapping for Ztso
ClosedPublic

Authored by reames on Feb 1 2023, 7:56 AM.

Details

Summary

This change implements a proposed lowering from LLVM's memory model to the TSO memory model defined by the Ztso extension. Selecting the proposed mapping turns out to be an involved conversation that really didn't fit within a review description, so let me refer you to https://github.com/preames/public-notes/blob/master/riscv-tso-mappings.rst. This review implements the WMO compatible variant (the proposed one in that document).

Ztso is currently accepted as an experimental extension in LLVM. Despite the fact the extension was recently ratified, I think we need to leave it as experimental until we have wide agreement on the chosen mapping for ABI purposes.

I need to note that the current in-tree implementation defaults to generating WMO compatible fences. This is entirely compatible with the proposed mapping in this patch, but is unfortunately not compatible with the major alternative. The in tree implementation is explicitly experimental so the impact of this is limited, but it is worth calling out that if settle on the alternative we will have a minor ABI break. My apologies for not calling this out in the original patch; I had not realized at the time that one of our realistic choices for mappings wouldn't be WMO compatible.

This patch only contains the changes for load/store and fence. That is, it does not change the lowering for atomicrmw operations. This is a sound thing to do under the proposed mapping since the existing WMO mappings remain compatible. I do plan to change these; I'm just working incrementally.

Diff Detail

Event Timeline

reames created this revision.Feb 1 2023, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 7:56 AM
reames requested review of this revision.Feb 1 2023, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 7:57 AM
asb added a comment.Feb 7 2023, 5:04 AM

Thanks for writing up such a clear summary of the options for this mapping. Thoughts on the conceptual change:

  • You're right to flag the ABI break, but I think we shouldn't worry about this - after all that's what the experimental flag is for.
  • Compatibility with RVWMO seems like such a big win to me it would need a _really_ strong evidence-backed argument to prefer the alternate mapping.
    • Perhaps not overly relevant to the current day discussion, but wonder to what extent this issue was considered within the original memory model working group. X86 "compatibility" was definitely raised explicitly, but I didn't interpret this to necessarily mean naive porting of X86 assembly (more e.g. DBTs)
  • There are enough mutually incompatible ABI variants for RISC-V already, I like the fact it seems like this lowering avoids introducing another one.

In terms of code review (all very minor points - it's a straightforward patch as all the work is clearly in the lowerings and the ecosystem tradeoffs mentioned above):

  • This needs an edit to RISCVUsage.rst (which currently says ztso doesn't yet change code generation)
  • I agree with the decision to just stick an if(ztso) at the beginning of the relevant methods vs trying to more tightly integrate the logic (which you could do in emitLeadingFence but I don't think it would be clearer and the LOC difference is negligible
  • You could have WMO/TSO/BOTH (or WMO/TSO/CHECK) check prefixes on atomic-fence.ll to make it easier to see when the lowerings are the same. Perhaps the same could be done in atomic-load-store.ll too (RV64IA-TSO, change the existing to RV64IA-WMO, and add an RV64IA-BOTH or similar).
reames updated this revision to Diff 495896.Feb 8 2023, 11:04 AM

Address review comments from @asb

asb accepted this revision.Feb 8 2023, 11:18 AM

The code changes LGTM.

As this has been floated in the RISC-V LLVM contributor call, and it's behind an experimental flag anyway (i.e. we can still change tack if future discussion shows there's huge opposition to the proposed lowering), I've got no opposition to merging this sooner rather than later and don't know of other ztso stakeholders working upstream who might be inconvenienced in either case.

Is your preference to try to get this merged in the near future, or would you rather wait for more feedback on the lowerings? (I'm pretty agnostic, for the reasons above).

This revision is now accepted and ready to land.Feb 8 2023, 11:18 AM

Is your preference to try to get this merged in the near future, or would you rather wait for more feedback on the lowerings? (I'm pretty agnostic, for the reasons above).

My preference is to merge, but it's not a hugely strong one.

asb added a comment.Feb 9 2023, 11:02 AM

Is your preference to try to get this merged in the near future, or would you rather wait for more feedback on the lowerings? (I'm pretty agnostic, for the reasons above).

My preference is to merge, but it's not a hugely strong one.

I'm happy for this to merge. It's been up for a week and discussed in the call with no concerns raised, and the whole point (from my perspective at least) of accepting patches for accepting patches under the experimental extensions flag was to enable better upstream collaboration rather than having things sit downstream.

craig.topper added inline comments.Feb 9 2023, 11:22 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3735

Was this whitespace change intentional?

3736

Drop curly braces

This revision was automatically updated to reflect the committed changes.