[PATCH] D143076: [RISCV] Implement a proposed mapping for Ztso

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.
Herald added a subscriber: jobnoorman.

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143076/new/

https://reviews.llvm.org/D143076