[lld] [llvm] [RISCV] Support RISCV Atomics ABI attributes (PR #84597)

Paul Kirth via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 2 09:26:30 PDT 2024


================
@@ -75,6 +75,13 @@ void RISCVTargetStreamer::emitTargetAttributes(const MCSubtargetInfo &STI,
     auto &ISAInfo = *ParseResult;
     emitTextAttribute(RISCVAttrs::ARCH, ISAInfo->toString());
   }
+
+  if (STI.hasFeature(RISCV::FeatureStdExtA)) {
+    unsigned AtomicABITag = STI.hasFeature(RISCV::FeatureTrailingSeqCstFence)
+                                ? RISCVAttrs::RISCVAtomicAbiTag::AtomicABI::A6S
----------------
ilovepi wrote:

> Is this the right way round? Isn't `STI.hasFeature(RISCV::FeatureTrailingSeqCstFence)` true if `+no-seq-cst-trailing-fence` is present? And if there's no trailing fence, then we have A6C? Or am I misreading?
> 

I think this should be correct. The default is now "enabled", so adding `+no-seq-cst-trailing-fence` should make  `STI.hasFeature(RISCV::FeatureTrailingSeqCstFence)` return `false`. I think the tests indicate that's how its working. 

> I think it would be better to name the feature `FeatureNoTrailingSeqCstFence` as we do for `FeatureNoRVCHints`. Though I think this patch would be best if it aimed to change no defaults at all, and left that as a follow-on patch (which would hopefully be a quick LGTM as I think there's no objection to that, going by this review thread).

Sure I'm happy to split this up into two patches. I'll try to get that out soon.



https://github.com/llvm/llvm-project/pull/84597


More information about the llvm-commits mailing list