From 190778a8ba6d30995b7e1b4b4a556ab6444bdf3a Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Sat, 16 Aug 2025 15:10:34 -0700 Subject: [PATCH 1/4] MCSymbol: Rename SymContents to kind The names "SymbolContents" and "SymContents*" members are confusing. Rename to kind and Kind::XXX similar to lld/ELF/Symbols.h Rename SymContentsVariable to Kind::Equated as the former term is "equated symbol", not "variable". --- llvm/include/llvm/MC/MCSymbol.h | 48 +++++++++++++++------------------ llvm/lib/MC/MCSymbol.cpp | 9 +++---- 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/llvm/include/llvm/MC/MCSymbol.h b/llvm/include/llvm/MC/MCSymbol.h index ce160bd2c3cc..740fae22d1b9 100644 --- a/llvm/include/llvm/MC/MCSymbol.h +++ b/llvm/include/llvm/MC/MCSymbol.h @@ -42,11 +42,11 @@ class raw_ostream; class MCSymbol { protected: // A symbol can be regular, equated to an expression, or a common symbol. - enum Contents : uint8_t { - SymContentsUnset, - SymContentsVariable, - SymContentsCommon, - SymContentsTargetCommon, // Index stores the section index + enum Kind : uint8_t { + Regular, + Equated, + Common, + TargetCommon, // Index stores the section index }; // Special sentinel value for the absolute pseudo fragment. @@ -65,9 +65,9 @@ protected: /// relative to, if any. mutable MCFragment *Fragment = nullptr; - /// This is actually a Contents enumerator, but is unsigned to avoid sign - /// extension and achieve better bitpacking with MSVC. - unsigned SymbolContents : 2; + /// The symbol kind. Use an unsigned bitfield to achieve better bitpacking + /// with MSVC. + unsigned kind : 2; /// True if this symbol is named. A named symbol will have a pointer to the /// name allocated in the bytes immediately prior to the MCSymbol. @@ -145,10 +145,10 @@ protected: }; MCSymbol(const MCSymbolTableEntry *Name, bool isTemporary) - : SymbolContents(SymContentsUnset), IsTemporary(isTemporary), - IsRedefinable(false), IsRegistered(false), IsExternal(false), - IsPrivateExtern(false), IsWeakExternal(false), IsUsedInReloc(false), - IsResolving(0), CommonAlignLog2(0), Flags(0) { + : kind(Kind::Regular), IsTemporary(isTemporary), IsRedefinable(false), + IsRegistered(false), IsExternal(false), IsPrivateExtern(false), + IsWeakExternal(false), IsUsedInReloc(false), IsResolving(0), + CommonAlignLog2(0), Flags(0) { Offset = 0; HasName = !!Name; if (Name) @@ -212,9 +212,9 @@ public: /// Prepare this symbol to be redefined. void redefineIfPossible() { if (IsRedefinable) { - if (SymbolContents == SymContentsVariable) { + if (kind == Kind::Equated) { Value = nullptr; - SymbolContents = SymContentsUnset; + kind = Kind::Regular; } setUndefined(); IsRedefinable = false; @@ -268,9 +268,7 @@ public: /// @{ /// isVariable - Check if this is a variable symbol. - bool isVariable() const { - return SymbolContents == SymContentsVariable; - } + bool isVariable() const { return kind == Equated; } /// Get the expression of the variable symbol. const MCExpr *getVariableValue() const { @@ -293,12 +291,12 @@ public: } uint64_t getOffset() const { - assert(SymbolContents == SymContentsUnset && + assert(kind == Kind::Regular && "Cannot get offset for a common/variable symbol"); return Offset; } void setOffset(uint64_t Value) { - assert(SymbolContents == SymContentsUnset && + assert(kind == Kind::Regular && "Cannot set offset for a common/variable symbol"); Offset = Value; } @@ -317,7 +315,7 @@ public: void setCommon(uint64_t Size, Align Alignment, bool Target = false) { assert(getOffset() == 0); CommonSize = Size; - SymbolContents = Target ? SymContentsTargetCommon : SymContentsCommon; + kind = Target ? Kind::TargetCommon : Kind::Common; unsigned Log2Align = encode(Alignment); assert(Log2Align < (1U << NumCommonAlignmentBits) && @@ -350,14 +348,12 @@ public: /// Is this a 'common' symbol. bool isCommon() const { - return SymbolContents == SymContentsCommon || - SymbolContents == SymContentsTargetCommon; + return kind == Kind::Common || kind == Kind::TargetCommon; } - /// Is this a target-specific common-like symbol. - bool isTargetCommon() const { - return SymbolContents == SymContentsTargetCommon; - } + /// Used by AMDGPU to indicate a common-like symbol of section index + /// SHN_AMDGPU_LDS. + bool isTargetCommon() const { return kind == Kind::TargetCommon; } MCFragment *getFragment() const { if (Fragment || !isVariable() || isWeakExternal()) diff --git a/llvm/lib/MC/MCSymbol.cpp b/llvm/lib/MC/MCSymbol.cpp index b19842aae46c..771b1204272d 100644 --- a/llvm/lib/MC/MCSymbol.cpp +++ b/llvm/lib/MC/MCSymbol.cpp @@ -48,12 +48,11 @@ void *MCSymbol::operator new(size_t s, const MCSymbolTableEntry *Name, } void MCSymbol::setVariableValue(const MCExpr *Value) { - assert(Value && "Invalid variable value!"); - assert((SymbolContents == SymContentsUnset || - SymbolContents == SymContentsVariable) && - "Cannot give common/offset symbol a variable value"); + assert(Value && "Invalid equated expression"); + assert((kind == Kind::Regular || kind == Kind::Equated) && + "Cannot equate a common symbol"); this->Value = Value; - SymbolContents = SymContentsVariable; + kind = Kind::Equated; setUndefined(); } From aa96e20dcefa7d73229c98a7d2727696ff949459 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Sat, 16 Aug 2025 15:39:33 -0700 Subject: [PATCH 2/4] MCSymbol: Remove AMDGPU-specific Kind::TargetCommon The SymContentsTargetCommon kind introduced by https://reviews.llvm.org/D61493 lackes significant and should be treated as a regular common symbol with a different section index. Update ELFObjectWriter to respect the specified section index. The new representation also works with Hexagon's SHN_HEXAGON_SCOMMON. --- llvm/include/llvm/MC/MCSymbol.h | 14 +++----------- llvm/lib/MC/ELFObjectWriter.cpp | 8 ++++---- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/llvm/include/llvm/MC/MCSymbol.h b/llvm/include/llvm/MC/MCSymbol.h index 740fae22d1b9..88e2230a0c34 100644 --- a/llvm/include/llvm/MC/MCSymbol.h +++ b/llvm/include/llvm/MC/MCSymbol.h @@ -46,7 +46,6 @@ protected: Regular, Equated, Common, - TargetCommon, // Index stores the section index }; // Special sentinel value for the absolute pseudo fragment. @@ -315,7 +314,7 @@ public: void setCommon(uint64_t Size, Align Alignment, bool Target = false) { assert(getOffset() == 0); CommonSize = Size; - kind = Target ? Kind::TargetCommon : Kind::Common; + kind = Kind::Common; unsigned Log2Align = encode(Alignment); assert(Log2Align < (1U << NumCommonAlignmentBits) && @@ -338,8 +337,7 @@ public: bool declareCommon(uint64_t Size, Align Alignment, bool Target = false) { assert(isCommon() || getOffset() == 0); if(isCommon()) { - if (CommonSize != Size || getCommonAlignment() != Alignment || - isTargetCommon() != Target) + if (CommonSize != Size || getCommonAlignment() != Alignment) return true; } else setCommon(Size, Alignment, Target); @@ -347,13 +345,7 @@ public: } /// Is this a 'common' symbol. - bool isCommon() const { - return kind == Kind::Common || kind == Kind::TargetCommon; - } - - /// Used by AMDGPU to indicate a common-like symbol of section index - /// SHN_AMDGPU_LDS. - bool isTargetCommon() const { return kind == Kind::TargetCommon; } + bool isCommon() const { return kind == Kind::Common; } MCFragment *getFragment() const { if (Fragment || !isVariable() || isWeakExternal()) diff --git a/llvm/lib/MC/ELFObjectWriter.cpp b/llvm/lib/MC/ELFObjectWriter.cpp index 8f3814a1dd62..759d3e0e1429 100644 --- a/llvm/lib/MC/ELFObjectWriter.cpp +++ b/llvm/lib/MC/ELFObjectWriter.cpp @@ -541,12 +541,12 @@ void ELFWriter::computeSymbolTable(const RevGroupMapTy &RevGroupMap) { if (Symbol.isAbsolute()) { MSD.SectionIndex = ELF::SHN_ABS; } else if (Symbol.isCommon()) { - if (Symbol.isTargetCommon()) { - MSD.SectionIndex = Symbol.getIndex(); - } else { + auto Shndx = Symbol.getIndex(); + if (!Shndx) { assert(!Local); - MSD.SectionIndex = ELF::SHN_COMMON; + Shndx = ELF::SHN_COMMON; } + MSD.SectionIndex = Shndx; } else if (Symbol.isUndefined()) { if (Symbol.isSignature() && !Symbol.isUsedInReloc()) { MSD.SectionIndex = RevGroupMap.lookup(&Symbol); From 2cedb286b8a37a3c6f09ac394b5e95413baac287 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Sat, 16 Aug 2025 15:47:39 -0700 Subject: [PATCH 3/4] MCSymbol: Remove unused IsTarget parameter from declareCommon --- llvm/include/llvm/MC/MCSymbol.h | 7 +++---- .../Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/llvm/include/llvm/MC/MCSymbol.h b/llvm/include/llvm/MC/MCSymbol.h index 88e2230a0c34..13d74157bc01 100644 --- a/llvm/include/llvm/MC/MCSymbol.h +++ b/llvm/include/llvm/MC/MCSymbol.h @@ -311,7 +311,7 @@ public: /// \param Size - The size of the symbol. /// \param Alignment - The alignment of the symbol. /// \param Target - Is the symbol a target-specific common-like symbol. - void setCommon(uint64_t Size, Align Alignment, bool Target = false) { + void setCommon(uint64_t Size, Align Alignment) { assert(getOffset() == 0); CommonSize = Size; kind = Kind::Common; @@ -332,15 +332,14 @@ public: /// /// \param Size - The size of the symbol. /// \param Alignment - The alignment of the symbol. - /// \param Target - Is the symbol a target-specific common-like symbol. /// \return True if symbol was already declared as a different type - bool declareCommon(uint64_t Size, Align Alignment, bool Target = false) { + bool declareCommon(uint64_t Size, Align Alignment) { assert(isCommon() || getOffset() == 0); if(isCommon()) { if (CommonSize != Size || getCommonAlignment() != Alignment) return true; } else - setCommon(Size, Alignment, Target); + setCommon(Size, Alignment); return false; } diff --git a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp index 1f35e92151bf..f000b2cc60c9 100644 --- a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp +++ b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp @@ -886,7 +886,7 @@ void AMDGPUTargetELFStreamer::emitAMDGPULDS(MCSymbol *Symbol, unsigned Size, if (!SymbolELF->isBindingSet()) SymbolELF->setBinding(ELF::STB_GLOBAL); - if (SymbolELF->declareCommon(Size, Alignment, true)) { + if (SymbolELF->declareCommon(Size, Alignment)) { report_fatal_error("Symbol: " + Symbol->getName() + " redeclared as different type"); } From 1f5047e43092f39a60a0ddba921610c2ab00897e Mon Sep 17 00:00:00 2001 From: Aiden Grossman Date: Sat, 16 Aug 2025 15:52:39 -0700 Subject: [PATCH 4/4] [Github] Remove call to llvm-project-tests.yml from spirv-tests.yml This will eventually allow for removing llvm-project-tests.yml. This should significantly reduce the complexity of these workflows at the cost of a little bit of duplication standard to github actions. Reviewers: michalpaszkowski, sudonatalie Reviewed By: sudonatalie Pull Request: https://github.com/llvm/llvm-project/pull/153869 --- .github/workflows/spirv-tests.yml | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/.github/workflows/spirv-tests.yml b/.github/workflows/spirv-tests.yml index f15ca1cb64ba..8708fb06d9eb 100644 --- a/.github/workflows/spirv-tests.yml +++ b/.github/workflows/spirv-tests.yml @@ -4,7 +4,6 @@ permissions: contents: read on: - workflow_dispatch: pull_request: paths: - 'llvm/lib/Target/SPIRV/**' @@ -21,9 +20,27 @@ jobs: check_spirv: if: github.repository_owner == 'llvm' name: Test SPIR-V - uses: ./.github/workflows/llvm-project-tests.yml - with: - build_target: check-llvm-codegen-spirv - projects: - extra_cmake_args: '-DLLVM_TARGETS_TO_BUILD="SPIRV" -DLLVM_INCLUDE_SPIRV_TOOLS_TESTS=ON' - os_list: '["ubuntu-24.04"]' + runs-on: ubuntu-24.04 + container: + image: ghcr.io/llvm/ci-ubuntu-24.04:latest + steps: + - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 + - name: Setup ccache + uses: hendrikmuhs/ccache-action@a1209f81afb8c005c13b4296c32e363431bffea5 # v1.2.17 + with: + max-size: 2G + key: spirv-ubuntu-24.04 + variant: sccache + - name: Build and Test + run: | + mkdir build + cmake -GNinja \ + -S llvm \ + -B build \ + -DCMAKE_BUILD_TYPE=Release \ + -DLLVM_ENABLE_ASSERTIONS=ON \ + -DCMAKE_C_COMPILER_LAUNCHER=sccache \ + -DCMAKE_CXX_COMPILER_LAUNCHER=sccache \ + -DLLVM_TARGETS_TO_BUILD="SPIRV" \ + -DLLVM_INCLUDE_SPIRV_TOOLS_TESTS=ON + ninja -C build check-llvm-codegen-spirv