[RISCV] Support RISCV Atomics ABI attributes #84597
Conversation
Created using spr 1.3.4
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-lld-elf Author: Paul Kirth (ilovepi) ChangesThis patch adds support for the The atomics_abi tag merging is conducted as follows:
Note: the A7 is not yet supported in either LLVM or in any current hardware, With the tag merging in place, we can safely change the default for Patch is 20.19 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84597.diff 13 Files Affected:
diff --git a/lld/ELF/Arch/RISCV.cpp b/lld/ELF/Arch/RISCV.cpp
index 4798c86f7d1b61..e44583fb80fd31 100644
--- a/lld/ELF/Arch/RISCV.cpp
+++ b/lld/ELF/Arch/RISCV.cpp
@@ -1084,10 +1084,88 @@ static void mergeArch(RISCVISAInfo::OrderedExtensionMap &mergedExts,
}
}
+static void mergeAtomic(DenseMap<unsigned, unsigned> &intAttr,
+ const InputSectionBase *oldSection,
+ const InputSectionBase *newSection,
+ unsigned int oldTag,
+ unsigned int newTag) {
+ using RISCVAttrs::RISCVAtomicAbiTag::AtomicABI;
+ llvm::errs() << "oldTag=" << oldTag << ", newTag=" << newTag <<"\n";
+ // Same tags stay the same, and UNKNOWN is compatible with anything
+ if (oldTag == newTag || newTag == AtomicABI::UNKNOWN)
+ return;
+
+ auto attr = RISCVAttrs::ATOMIC_ABI;
+ switch (oldTag) {
+ case AtomicABI::UNKNOWN:
+ intAttr[attr] = newTag;
+ return;
+ case AtomicABI::A6C:
+ switch (newTag) {
+ case AtomicABI::A6S:
+ intAttr[attr] = AtomicABI::A6C;
+ return;
+ case AtomicABI::A7:
+ errorOrWarn(toString(oldSection) + " has atomic_abi=" + Twine(oldTag) +
+ " but " + toString(newSection) +
+ " has atomic_abi=" + Twine(newTag));
+ return;
+ };
+
+ case AtomicABI::A6S:
+ switch (newTag) {
+ case AtomicABI::A6C:
+ intAttr[attr] = AtomicABI::A6C;
+ return;
+ case AtomicABI::A7:
+ intAttr[attr] = AtomicABI::A7;
+ return;
+ };
+
+ case AtomicABI::A7:
+ switch (newTag) {
+ case AtomicABI::A6S:
+ intAttr[attr] = AtomicABI::A7;
+ return;
+ case AtomicABI::A6C:
+ errorOrWarn(toString(oldSection) + " has atomic_abi=" + Twine(oldTag) +
+ " but " + toString(newSection) +
+ " has atomic_abi=" + Twine(newTag));
+ return;
+ };
+ default:
+ llvm_unreachable("unknown AtomicABI");
+ };
+}
+
+static void mergeX3RegUse(DenseMap<unsigned, unsigned> &intAttr,
+ const InputSectionBase *oldSection,
+ const InputSectionBase *newSection,
+ unsigned int oldTag,
+ unsigned int newTag) {
+ // X3/GP register usage ar incompatible and cannot be merged, with the
+ // exception of the UNKNOWN or 0 value
+ using RISCVAttrs::RISCVX3RegUse::X3RegUsage;
+ auto attr = RISCVAttrs::X3_REG_USAGE;
+ if (newTag == X3RegUsage::UNKNOWN)
+ return;
+ if (oldTag == X3RegUsage::UNKNOWN)
+ intAttr[attr] = newTag;
+ if (oldTag != newTag) {
+ errorOrWarn(toString(oldSection) + " has x3_reg_usage=" + Twine(oldTag) +
+ " but " + toString(newSection) +
+ " has x3_reg_usage=" + Twine(newTag));
+ return;
+ }
+ // TODO: do we need to check the tags are < 2047?
+}
+
static RISCVAttributesSection *
mergeAttributesSection(const SmallVector<InputSectionBase *, 0> §ions) {
RISCVISAInfo::OrderedExtensionMap exts;
const InputSectionBase *firstStackAlign = nullptr;
+ const InputSectionBase *firstAtomicAbi = nullptr;
+ const InputSectionBase *firstX3RegUse = nullptr;
unsigned firstStackAlignValue = 0, xlen = 0;
bool hasArch = false;
@@ -1134,6 +1212,18 @@ mergeAttributesSection(const SmallVector<InputSectionBase *, 0> §ions) {
case RISCVAttrs::PRIV_SPEC_MINOR:
case RISCVAttrs::PRIV_SPEC_REVISION:
break;
+
+ case llvm::RISCVAttrs::AttrType::ATOMIC_ABI:
+ if (auto i = parser.getAttributeValue(tag.attr)) {
+ auto r = merged.intAttr.try_emplace(tag.attr, *i);
+ if (r.second) {
+ firstAtomicAbi = sec;
+ } else {
+ mergeAtomic(merged.intAttr, firstAtomicAbi, sec, r.first->getSecond(), *i);
+ llvm::errs() << "Merged Attr = " <<merged.intAttr[tag.attr] << "\n";
+ }
+ }
+ continue;
}
// Fallback for deprecated priv_spec* and other unknown attributes: retain
diff --git a/lld/test/ELF/riscv-attributes.s b/lld/test/ELF/riscv-attributes.s
index d0ce0941269ec4..964fec43b52847 100644
--- a/lld/test/ELF/riscv-attributes.s
+++ b/lld/test/ELF/riscv-attributes.s
@@ -44,6 +44,30 @@
# RUN: not ld.lld a.o b.o c.o diff_stack_align.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=STACK_ALIGN --implicit-check-not=error:
# STACK_ALIGN: error: diff_stack_align.o:(.riscv.attributes) has stack_align=32 but a.o:(.riscv.attributes) has stack_align=16
+## merging atomic_abi values for A6C and A7 lead to an error.
+# RUN: llvm-mc -filetype=obj -triple=riscv64 atomic_abi_A6C.s -o atomic_abi_A6C.o
+# RUN: llvm-mc -filetype=obj -triple=riscv64 atomic_abi_A7.s -o atomic_abi_A7.o
+# RUN: not ld.lld atomic_abi_A6C.o atomic_abi_A7.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ATOMIC_ABI_ERROR --implicit-check-not=error:
+# ATOMIC_ABI_ERROR: error: atomic_abi_A6C.o:(.riscv.attributes) has atomic_abi=1 but atomic_abi_A7.o:(.riscv.attributes) has atomic_abi=3
+
+
+# RUN: llvm-mc -filetype=obj -triple=riscv64 atomic_abi_A6S.s -o atomic_abi_A6S.o
+# RUN: ld.lld atomic_abi_A6S.o atomic_abi_A6C.o -o atomic_abi_A6C_A6S
+# RUN: llvm-readobj -A atomic_abi_A6C_A6S | FileCheck %s --check-prefix=A6C_A6S
+
+# RUN: ld.lld atomic_abi_A6S.o atomic_abi_A7.o -o atomic_abi_A6S_A7
+# RUN: llvm-readobj -A atomic_abi_A6S_A7 | FileCheck %s --check-prefix=A6S_A7
+
+# RUN: llvm-mc -filetype=obj -triple=riscv64 atomic_abi_unknown.s -o atomic_abi_unknown.o
+# RUN: ld.lld atomic_abi_unknown.o atomic_abi_A6C.o -o atomic_abi_A6C_unknown
+# RUN: llvm-readobj -A atomic_abi_A6C_unknown | FileCheck %s --check-prefixes=UNKNOWN_A6C
+
+# RUN: ld.lld atomic_abi_unknown.o atomic_abi_A6S.o -o atomic_abi_A6S_unknown
+# RUN: llvm-readobj -A atomic_abi_A6S_unknown | FileCheck %s --check-prefix=UNKNOWN_A6S
+
+# RUN: ld.lld atomic_abi_unknown.o atomic_abi_A7.o -o atomic_abi_A7_unknown
+# RUN: llvm-readobj -A atomic_abi_A7_unknown | FileCheck %s --check-prefix=UNKNOWN_A7
+
## The deprecated priv_spec is not handled as GNU ld does.
## Differing priv_spec attributes lead to an absent attribute.
# RUN: llvm-mc -filetype=obj -triple=riscv64 diff_priv_spec.s -o diff_priv_spec.o
@@ -286,6 +310,108 @@
.attribute priv_spec, 3
.attribute priv_spec_minor, 3
+#--- atomic_abi_unknown.s
+.attribute atomic_abi, 0
+
+#--- atomic_abi_A6C.s
+.attribute atomic_abi, 1
+
+#--- atomic_abi_A6S.s
+.attribute atomic_abi, 2
+
+#--- atomic_abi_A7.s
+.attribute atomic_abi, 3
+
+# UNKNOWN_A6C: BuildAttributes {
+# UNKNOWN_A6C: FormatVersion: 0x41
+# UNKNOWN_A6C: Section 1 {
+# UNKNOWN_A6C: SectionLength: 17
+# UNKNOWN_A6C: Vendor: riscv
+# UNKNOWN_A6C: Tag: Tag_File (0x1)
+# UNKNOWN_A6C: Size: 7
+# UNKNOWN_A6C: FileAttributes {
+# UNKNOWN_A6C: Attribute {
+# UNKNOWN_A6C: Tag: 14
+# UNKNOWN_A6C: Value: 1
+# UNKNOWN_A6C: TagName: atomic_abi
+# UNKNOWN_A6C: Description: Atomic ABI is 1
+# UNKNOWN_A6C: }
+# UNKNOWN_A6C: }
+# UNKNOWN_A6C: }
+# UNKNOWN_A6C: }
+
+# UNKNOWN_A6S: BuildAttributes {
+# UNKNOWN_A6S: FormatVersion: 0x41
+# UNKNOWN_A6S: Section 1 {
+# UNKNOWN_A6S: SectionLength: 17
+# UNKNOWN_A6S: Vendor: riscv
+# UNKNOWN_A6S: Tag: Tag_File (0x1)
+# UNKNOWN_A6S: Size: 7
+# UNKNOWN_A6S: FileAttributes {
+# UNKNOWN_A6S: Attribute {
+# UNKNOWN_A6S: Tag: 14
+# UNKNOWN_A6S: Value: 2
+# UNKNOWN_A6S: TagName: atomic_abi
+# UNKNOWN_A6S: Description: Atomic ABI is 2
+# UNKNOWN_A6S: }
+# UNKNOWN_A6S: }
+# UNKNOWN_A6S: }
+# UNKNOWN_A6S: }
+
+# UNKNOWN_A7: BuildAttributes {
+# UNKNOWN_A7: FormatVersion: 0x41
+# UNKNOWN_A7: Section 1 {
+# UNKNOWN_A7: SectionLength: 17
+# UNKNOWN_A7: Vendor: riscv
+# UNKNOWN_A7: Tag: Tag_File (0x1)
+# UNKNOWN_A7: Size: 7
+# UNKNOWN_A7: FileAttributes {
+# UNKNOWN_A7: Attribute {
+# UNKNOWN_A7: Tag: 14
+# UNKNOWN_A7: Value: 3
+# UNKNOWN_A7: TagName: atomic_abi
+# UNKNOWN_A7: Description: Atomic ABI is 3
+# UNKNOWN_A7: }
+# UNKNOWN_A7: }
+# UNKNOWN_A7: }
+# UNKNOWN_A7: }
+
+# A6C_A6S: BuildAttributes {
+# A6C_A6S: FormatVersion: 0x41
+# A6C_A6S: Section 1 {
+# A6C_A6S: SectionLength: 17
+# A6C_A6S: Vendor: riscv
+# A6C_A6S: Tag: Tag_File (0x1)
+# A6C_A6S: Size: 7
+# A6C_A6S: FileAttributes {
+# A6C_A6S: Attribute {
+# A6C_A6S: Tag: 14
+# A6C_A6S: Value: 1
+# A6C_A6S: TagName: atomic_abi
+# A6C_A6S: Description: Atomic ABI is 1
+# A6C_A6S: }
+# A6C_A6S: }
+# A6C_A6S: }
+# A6C_A6S: }
+
+# A6S_A7: BuildAttributes {
+# A6S_A7: FormatVersion: 0x41
+# A6S_A7: Section 1 {
+# A6S_A7: SectionLength: 17
+# A6S_A7: Vendor: riscv
+# A6S_A7: Tag: Tag_File (0x1)
+# A6S_A7: Size: 7
+# A6S_A7: FileAttributes {
+# A6S_A7: Attribute {
+# A6S_A7: Tag: 14
+# A6S_A7: Value: 3
+# A6S_A7: TagName: atomic_abi
+# A6S_A7: Description: Atomic ABI is 3
+# A6S_A7: }
+# A6S_A7: }
+# A6S_A7: }
+# A6S_A7: }
+
#--- unknown13.s
.attribute 13, "0"
#--- unknown13a.s
diff --git a/llvm/include/llvm/Support/RISCVAttributeParser.h b/llvm/include/llvm/Support/RISCVAttributeParser.h
index 305adffbe851e7..9f295504de959e 100644
--- a/llvm/include/llvm/Support/RISCVAttributeParser.h
+++ b/llvm/include/llvm/Support/RISCVAttributeParser.h
@@ -24,6 +24,7 @@ class RISCVAttributeParser : public ELFAttributeParser {
Error unalignedAccess(unsigned tag);
Error stackAlign(unsigned tag);
+ Error atomicAbi(unsigned tag);
public:
RISCVAttributeParser(ScopedPrinter *sw)
diff --git a/llvm/include/llvm/Support/RISCVAttributes.h b/llvm/include/llvm/Support/RISCVAttributes.h
index 18f5a84d21f250..5def890a727355 100644
--- a/llvm/include/llvm/Support/RISCVAttributes.h
+++ b/llvm/include/llvm/Support/RISCVAttributes.h
@@ -32,8 +32,21 @@ enum AttrType : unsigned {
PRIV_SPEC = 8,
PRIV_SPEC_MINOR = 10,
PRIV_SPEC_REVISION = 12,
+ ATOMIC_ABI = 14,
};
+namespace RISCVAtomicAbiTag {
+enum AtomicABI : unsigned {
+ // Values for Tag_RISCV_atomic_abi
+ // Defined at
+ // https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#tag_riscv_atomic_abi-14-uleb128version
+ UNKNOWN = 0,
+ A6C = 1,
+ A6S = 2,
+ A7 = 3,
+};
+} // namespace RISCVAtomicAbiTag
+
enum { NOT_ALLOWED = 0, ALLOWED = 1 };
} // namespace RISCVAttrs
diff --git a/llvm/lib/Support/RISCVAttributeParser.cpp b/llvm/lib/Support/RISCVAttributeParser.cpp
index 7ce4b6ab161cd3..b9515134181edb 100644
--- a/llvm/lib/Support/RISCVAttributeParser.cpp
+++ b/llvm/lib/Support/RISCVAttributeParser.cpp
@@ -36,7 +36,23 @@ const RISCVAttributeParser::DisplayHandler
{
RISCVAttrs::UNALIGNED_ACCESS,
&RISCVAttributeParser::unalignedAccess,
- }};
+ },
+ {
+ RISCVAttrs::ATOMIC_ABI,
+ &RISCVAttributeParser::atomicAbi,
+ },
+ {
+ RISCVAttrs::X3_REG_USAGE,
+ &RISCVAttributeParser::x3RegUsage,
+ },
+};
+
+Error RISCVAttributeParser::atomicAbi(unsigned Tag) {
+ uint64_t Value = de.getULEB128(cursor);
+ std::string Description = "Atomic ABI is " + utostr(Value);
+ printAttribute(Tag, Value, Description);
+ return Error::success();
+}
Error RISCVAttributeParser::unalignedAccess(unsigned tag) {
static const char *strings[] = {"No unaligned access", "Unaligned access"};
diff --git a/llvm/lib/Support/RISCVAttributes.cpp b/llvm/lib/Support/RISCVAttributes.cpp
index 9e629760d3d842..dc70d65acba063 100644
--- a/llvm/lib/Support/RISCVAttributes.cpp
+++ b/llvm/lib/Support/RISCVAttributes.cpp
@@ -18,6 +18,7 @@ static constexpr TagNameItem tagData[] = {
{PRIV_SPEC, "Tag_priv_spec"},
{PRIV_SPEC_MINOR, "Tag_priv_spec_minor"},
{PRIV_SPEC_REVISION, "Tag_priv_spec_revision"},
+ {ATOMIC_ABI, "Tag_atomic_abi"},
};
constexpr TagNameMap RISCVAttributeTags{tagData};
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
index 4a4b1e13c2b9ec..3f4d6921841a27 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -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
+ : RISCVAttrs::RISCVAtomicAbiTag::AtomicABI::A6C;
+ emitAttribute(RISCVAttrs::ATOMIC_ABI, AtomicABITag);
+ }
}
// This part is for ascii assembly output
diff --git a/llvm/lib/Target/RISCV/RISCVFeatures.td b/llvm/lib/Target/RISCV/RISCVFeatures.td
index 83619ccb24baa3..e3ce527b84a985 100644
--- a/llvm/lib/Target/RISCV/RISCVFeatures.td
+++ b/llvm/lib/Target/RISCV/RISCVFeatures.td
@@ -1154,10 +1154,10 @@ foreach i = {1-31} in
def FeatureSaveRestore : SubtargetFeature<"save-restore", "EnableSaveRestore",
"true", "Enable save/restore.">;
-def FeatureTrailingSeqCstFence : SubtargetFeature<"seq-cst-trailing-fence",
+def FeatureTrailingSeqCstFence : SubtargetFeature<"no-seq-cst-trailing-fence",
"EnableSeqCstTrailingFence",
- "true",
- "Enable trailing fence for seq-cst store.">;
+ "false",
+ "Disable trailing fence for seq-cst store.">;
def FeatureFastUnalignedAccess
: SubtargetFeature<"fast-unaligned-access", "HasFastUnalignedAccess",
diff --git a/llvm/test/CodeGen/RISCV/atomic-load-store.ll b/llvm/test/CodeGen/RISCV/atomic-load-store.ll
index 2d1fc21cda89b0..30680957ecbc6b 100644
--- a/llvm/test/CodeGen/RISCV/atomic-load-store.ll
+++ b/llvm/test/CodeGen/RISCV/atomic-load-store.ll
@@ -1,26 +1,26 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefix=RV32I %s
-; RUN: llc -mtriple=riscv32 -mattr=+a -verify-machineinstrs < %s \
+; RUN: llc -mtriple=riscv32 -mattr=+a,+no-seq-cst-trailing-fence -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefixes=RV32IA,RV32IA-WMO %s
-; RUN: llc -mtriple=riscv32 -mattr=+a,+experimental-ztso -verify-machineinstrs < %s \
+; RUN: llc -mtriple=riscv32 -mattr=+a,+no-seq-cst-trailing-fence,+experimental-ztso -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefixes=RV32IA,RV32IA-TSO %s
; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefix=RV64I %s
-; RUN: llc -mtriple=riscv64 -mattr=+a -verify-machineinstrs < %s \
+; RUN: llc -mtriple=riscv64 -mattr=+a,+no-seq-cst-trailing-fence -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefixes=RV64IA,RV64IA-WMO %s
-; RUN: llc -mtriple=riscv64 -mattr=+a,+experimental-ztso -verify-machineinstrs < %s \
+; RUN: llc -mtriple=riscv64 -mattr=+a,+no-seq-cst-trailing-fence,+experimental-ztso -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefixes=RV64IA,RV64IA-TSO %s
-; RUN: llc -mtriple=riscv32 -mattr=+a,+seq-cst-trailing-fence -verify-machineinstrs < %s \
+; RUN: llc -mtriple=riscv32 -mattr=+a -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefixes=RV32IA,RV32IA-WMO-TRAILING-FENCE %s
-; RUN: llc -mtriple=riscv32 -mattr=+a,+experimental-ztso,+seq-cst-trailing-fence -verify-machineinstrs < %s \
+; RUN: llc -mtriple=riscv32 -mattr=+a,+experimental-ztso -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefixes=RV32IA,RV32IA-TSO-TRAILING-FENCE %s
-; RUN: llc -mtriple=riscv64 -mattr=+a,+seq-cst-trailing-fence -verify-machineinstrs < %s \
+; RUN: llc -mtriple=riscv64 -mattr=+a -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefixes=RV64IA,RV64IA-WMO-TRAILING-FENCE %s
-; RUN: llc -mtriple=riscv64 -mattr=+a,+experimental-ztso,+seq-cst-trailing-fence -verify-machineinstrs < %s \
+; RUN: llc -mtriple=riscv64 -mattr=+a,+experimental-ztso -verify-machineinstrs < %s \
; RUN: | FileCheck -check-prefixes=RV64IA,RV64IA-TSO-TRAILING-FENCE %s
diff --git a/llvm/test/CodeGen/RISCV/attributes.ll b/llvm/test/CodeGen/RISCV/attributes.ll
index cc332df271043f..95bce2ae8a071a 100644
--- a/llvm/test/CodeGen/RISCV/attributes.ll
+++ b/llvm/test/CodeGen/RISCV/attributes.ll
@@ -128,7 +128,8 @@
; RUN: llc -mtriple=riscv64 -mattr=+m %s -o - | FileCheck --check-prefixes=CHECK,RV64M %s
; RUN: llc -mtriple=riscv64 -mattr=+zmmul %s -o - | FileCheck --check-prefixes=CHECK,RV64ZMMUL %s
; RUN: llc -mtriple=riscv64 -mattr=+m,+zmmul %s -o - | FileCheck --check-prefixes=CHECK,RV64MZMMUL %s
-; RUN: llc -mtriple=riscv64 -mattr=+a %s -o - | FileCheck --check-prefixes=CHECK,RV64A %s
+; RUN: llc -mtriple=riscv64 -mattr=+a %s -o - | FileCheck --check-prefixes=CHECK,RV64A,A6C %s
+; RUN: llc -mtriple=riscv64 -mattr=+a,+no-seq-cst-trailing-fence %s -o - | FileCheck --check-prefixes=CHECK,RV64A,A6S %s
; RUN: llc -mtriple=riscv64 -mattr=+f %s -o - | FileCheck --check-prefixes=CHECK,RV64F %s
; RUN: llc -mtriple=riscv64 -mattr=+d %s -o - | FileCheck --check-prefixes=CHECK,RV64D %s
; RUN: llc -mtriple=riscv64 -mattr=+c %s -o - | FileCheck --check-prefixes=CHECK,RV64C %s
@@ -512,3 +513,10 @@ define i32 @addi(i32 %a) {
%1 = add i32 %a, 1
ret i32 %1
}
+
+define i8 @atomic_load_i8_seq_cst(ptr %a) nounwind {
+ %1 = load atomic i8, ptr %a seq_cst, align 1
+ ret i8 %1
+; A6S: .attribute 14, 2
+; A6C: .attribute 14, 1
+}
diff --git a/llvm/test/CodeGen/RISCV/forced-atomics.ll b/llvm/test/CodeGen/RISCV/forced-atomics.ll
index f6a53a9d76dd35..96d3eede524510 100644
--- a/llvm/test/CodeGen/RISCV/forced-atomics.ll
+++ b/llvm/test/CodeGen/RISCV/forced-atomics.ll
@@ -1,12 +1,12 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=riscv32 -mattr=+seq-cst-trailing-fence < %s | FileCheck %s --check-prefixes=RV32,RV32-NO-ATOMIC
+; RUN: llc -mtriple=riscv32 -mattr=+no-seq-cst-trailing-fence < %s | FileCheck %s --check-prefixes=RV32,RV32-NO-ATOMIC
; RUN: llc -mtriple=riscv32 < %s | FileCheck %s --check-prefixes=RV32,RV32-NO-ATOMIC
-; RUN: llc -mtriple=riscv32 -mattr=+forced-atomics < %s | FileCheck %s --check-prefixes=RV32,RV32-ATOMIC
-; RUN: llc -mtriple=riscv32 -mattr=+forced-atomics,+seq-cst-trailing-fence < %s | FileCheck %s --check-prefixes=RV32,RV32-ATOMIC-TRAILING
+; RUN: llc -mtriple=riscv32 -mattr=+forced-atomics,+no-seq-cst-trailing-fence < %s | FileCheck %s --check-prefixes=RV32,RV32-ATOMIC
+; RUN: llc -mtriple=riscv32 -mattr=+forced-atomics < %s | FileCheck %s --check-prefixes=RV32,RV32-ATOMIC-TRAILING
; RUN: llc -mtriple=riscv64 < %s | FileCheck %s --check-prefixes=RV64,RV64-NO-ATOMIC
-; RUN: llc -mtriple=riscv64 -mattr=+seq-cst-trailing-fence < %s | FileCheck %s --check-prefixes=RV64,RV64-NO-ATOMIC
-; RUN: llc -mtriple=riscv64 -mattr=+forced-atomics < %s | FileCheck %s --check-prefixes=RV64,RV64-ATOMIC
-; RUN: llc -mtriple=riscv64 -mattr=+forced-atomics,+seq-cst-trailing-fence < %s | FileCheck %s --check-prefixes=RV64,RV64-ATOMIC-TRAILING
+; RUN: llc -mtriple=riscv64 -mattr=+no-seq-cst-trailing-fence < %s | FileCheck %s --check-prefixes=RV64,RV64-NO-ATOMIC
+; RUN: llc -mtriple=riscv64 -mattr=+forced-atomics,+no-seq-cst-trailing-fence < %s | FileCheck %s --check-prefixes=RV64,RV64-ATOMIC
+; RUN: llc -mtriple=riscv64 -mattr=+forced-atomics < %s | FileCheck %s --check-prefixes=RV64,RV64-ATOMIC-TRAILING
define i8 @load8(ptr %p) nounwind {
; RV32-NO-ATOMIC-LABEL: load8:
diff --git a/llvm/test/MC/RISCV/attribute.s b/llvm/test/MC/RISCV/attribute.s
index 56f0cb1daf176f..0a9d86da55261c 100644
--- a/llvm/test/MC/RISCV/attribute.s
+++ b/llvm/test/MC/RISCV/attribute.s
@@ -24,3 +24,6 @@
.attribute priv_spec_revision, 0
# CHECK: attribute 12, 0
+.attribute atomic_abi, 0
+# CHECK: attribute 14, 0
+
diff --git a/llvm/test/MC/RISCV/invalid-attribute.s b/llvm/test/MC/RISCV/invalid-attribute.s
index 1d732af83cda35..2ebf7ddc9aff85 100644
--- a/llvm/test/MC/RISCV/invalid-attribute.s
+++ b/llvm/test/MC/RISCV/invalid-attribute...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Created using spr 1.3.4
lld/ELF/Arch/RISCV.cpp
Outdated
| const InputSectionBase *newSection, unsigned int oldTag, | ||
| unsigned int newTag) { | ||
| using RISCVAttrs::RISCVAtomicAbiTag::AtomicABI; | ||
| llvm::errs() << "oldTag=" << oldTag << ", newTag=" << newTag << "\n"; |
There was a problem hiding this comment.
Oh I missed that. Thanks.
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4
lld/ELF/Arch/RISCV.cpp
Outdated
| auto attr = RISCVAttrs::ATOMIC_ABI; | ||
| switch (oldTag) { | ||
| case AtomicABI::UNKNOWN: | ||
| intAttr[attr] = newTag; |
There was a problem hiding this comment.
Can we use the original iterator returned by try_emplace to avoid the second map lookup here?
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4
|
@jrtc27 Do you have thoughts on this change w.r.t. the defaults? I recall you had expressed concerns previously in the psABI, so I want to be sure we're changing the status quo appropriately. |
To be clear, without reading the patch in detail, this is making the default be A6S? I don't think I have concerns with that other than to note that transitioning from A6S to A7 will be an ABI break for existing binaries (predating the use of A6S), so the timeline for when that's ok to do is unclear. |
Created using spr 1.3.4
|
ping. I think everything has been addressed. |
Created using spr 1.3.4
|
Thanks for splitting out that change. I'm thinking you might be missing a test case in lld/test/ELF/riscv-attributes.s. You handle an input file where the atomic_abi attribute is present and explicitly set to |
Oh, that's a good catch. I didn't think about that case. Let me see what happens. |
Created using spr 1.3.4
lld/test/ELF/riscv-attributes.s
Outdated
| @@ -62,12 +62,21 @@ | |||
| # RUN: ld.lld atomic_abi_unknown.o atomic_abi_A6C.o -o atomic_abi_A6C_unknown | |||
| # RUN: llvm-readobj -A atomic_abi_A6C_unknown | FileCheck %s --check-prefixes=UNKNOWN_A6C | |||
|
|
|||
| # RUN: ld.lld atomic_abi_unknown.o diff_stack_align.o -o atomic_abi_none_unknown | |||
| # RUN: llvm-readobj -A atomic_abi_none_unknown | FileCheck %s --check-prefixes=UNKNOWN_UNKNOWN | |||
There was a problem hiding this comment.
Shouldn't the check-prefix for this be NONE_UNKNOWN to match the convention elsewhere?
lld/test/ELF/riscv-attributes.s
Outdated
| # RUN: ld.lld atomic_abi_unknown.o diff_stack_align.o -o atomic_abi_none_unknown | ||
| # RUN: llvm-readobj -A atomic_abi_none_unknown | FileCheck %s --check-prefixes=UNKNOWN_UNKNOWN | ||
|
|
||
| # RUN: ld.lld diff_stack_align.o atomic_abi_A6C.o -o atomic_abi_A6C_unknown |
There was a problem hiding this comment.
Shouldn't the output filename be atomic_abi_A6C_none?
lld/test/ELF/riscv-attributes.s
Outdated
| @@ -322,6 +331,49 @@ | |||
| #--- atomic_abi_A7.s | |||
| .attribute atomic_abi, 3 | |||
|
|
|||
|
|
|||
| # UNKNOWN_UNKNOWN: BuildAttributes { | |||
There was a problem hiding this comment.
My other two comments were minor nits, this is the one where I'm wondering if there's some missing logic.
For every other case where one of the input files has the atomic_abi attribute and we produce an output, the output file also has the attribute. In this case, although one of the inputs had the attribute we end up dropping it in the output. I think that's probably not desired, but maybe I'm missing a reason to do this?
There was a problem hiding this comment.
So, we probably need to double check w/ @MaskRay, but I believe we don't emit attributes w/ 0 value in general. I think the following snippet seems to indicate that from a few lines below the call to mergeAtomics. I assume that is because the default value of 0 is pretty much 'not used', so it isn't emitted normally, and since UNKOWN is 0, it just falls into that case.
size_t size = 5 + merged.vendor.size() + 1 + 5;
for (auto &attr : merged.intAttr)
if (attr.second != 0)
size += getULEB128Size(attr.first) + getULEB128Size(attr.second);
I'll update the patch to address the nits, but I'll try to wait for Fangrui to confirm if thats correct about 0 valued attributes before proceeding.
There was a problem hiding this comment.
The main reason it might be helpful to differentiate between the attribute being present but UNKNOWN and the attribute not being present at all, is that if the attribute is there (and emitted when merging UNKNOWN with NONE) you at least know it's been processed by tools that were aware of the attribute. Is that useful? Not sure. It's probably not hugely important either way, but thought I'd check if this was by design or not.
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4
ilovepi
deleted the
users/ilovepi/spr/riscv-support-riscv-atomics-abi-attributes
branch
|
Hi, I'm seeing a segfault with GNU ld after this when running clang on a simple C example with #include <stdio.h>
int main() {
puts("hello");
}With LLD 18.1.3 I get a warning: I haven't tried it with a ToT lld yet but I thought I'd flag this in case something was accidentally made backwards incompatible! |
…fault (#87376)" This reverts commit 733b271. Reverting in order to revert the companion patch adding the atomics ABI ELF attributes due to the reported incompatibility with GNU ld. #84597 (comment)
This reverts commit 9221f3a. As reported <#84597 (comment)> and confirmed by me locally, adding these attributes causes current GNU ld to segfault when processing the input. Reverted so we can discuss the best next step.
|
Thanks Luke. I can reproduce locally (with GNU ld 2.42) so have gone ahead and reverted so we can decide what to do without breaking people's workflows in the meantime. CC @kito-cheng It seems like there are two problems here, the first obviously by far the biggest:
|
| it->getSecond() = AtomicABI::A6C; | ||
| return; | ||
| case AtomicABI::A7: | ||
| error(toString(oldSection) + " has atomic_abi=" + Twine(oldTag) + |
There was a problem hiding this comment.
This should use errorOrWarn.
Sorry that I did not follow up the discussion in time, but I'd appreciate that I had the opportunity to give this comment after the patch was approved by @asb ...
There was a problem hiding this comment.
Noted. I'll follow up in #90266. I based this on your comment to a similar line in #84598 (comment), but it seems like I misunderstood?
| # RUN: llvm-mc -filetype=obj -triple=riscv64 atomic_abi_A7.s -o atomic_abi_A7.o | ||
| # RUN: not ld.lld atomic_abi_A6C.o atomic_abi_A7.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ATOMIC_ABI_ERROR --implicit-check-not=error: | ||
| # ATOMIC_ABI_ERROR: error: atomic_abi_A6C.o:(.riscv.attributes) has atomic_abi=1 but atomic_abi_A7.o:(.riscv.attributes) has atomic_abi=3 | ||
|
|
There was a problem hiding this comment.
We don't use double blank lines
| # ATOMIC_ABI_ERROR: error: atomic_abi_A6C.o:(.riscv.attributes) has atomic_abi=1 but atomic_abi_A7.o:(.riscv.attributes) has atomic_abi=3 | ||
|
|
||
|
|
||
| # RUN: llvm-mc -filetype=obj -triple=riscv64 atomic_abi_A6S.s -o atomic_abi_A6S.o |
| @@ -44,6 +44,39 @@ | |||
| # RUN: not ld.lld a.o b.o c.o diff_stack_align.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=STACK_ALIGN --implicit-check-not=error: | |||
| # STACK_ALIGN: error: diff_stack_align.o:(.riscv.attributes) has stack_align=32 but a.o:(.riscv.attributes) has stack_align=16 | |||
|
|
|||
| ## merging atomic_abi values for A6C and A7 lead to an error. | |||
There was a problem hiding this comment.
Mismatched
Upper case. And there is a better word than "merging"
I am sorry it took a long time for me to notice the discussion and make some comments about the linker code. |
Apologies. That wasn't my intent. I'll be sure to allow some more time in the future. |
Sorry for that @MaskRay. I should have given an LGTM conditional on you or another lld dev explicitly lgtming the linker code. I think given jrtc27 had been involved in the thread and a discussion thread with you was mentioned I'd misjudged the level of attention this had got already from lld folks - so my bad. |
This patch adds support for the
atomic_abiattribute, specifid inhttps://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#tag_riscv_atomic_abi-14-uleb128version.
The atomics_abi tag merging is conducted as follows:
Note: the A7 is not yet supported in either LLVM or in any current hardware,
and is therefore ommited from attribute generation in RISCVTargetStreamer.