[MC][DecoderEmitter] Fix build warning: explicit specialization cannot have a storage class

[jurahul]

Move InsnBitWidth template into anonymous namespace in the generated code and move template specialization of InsnBitWidth to anonymous namespace as well, and drop static for them. This makes InsnBitWidth completely private to each target and fixes the "explicit specialization cannot have a storage class" warning as well as any potential linker errors if InsnBitWidth is kept in the llvm::MCD namespace.

[jurahul]

Seems to build locally for me.

[jurahul]

Will wait for checks before committing

[jurahul]

:( That does not work. What seems to work is moving InsnBitWidth into an anonymous namespace in the generated code and also moving all the specializations into anonymous namespace. Testing that now locally

[jurahul]

Attempt #2..lets see how it goes

[s-barannikov]

:( That does not work.

What are the symptoms? Not sure what difference does anonymous namespace make.

[jurahul]

:( That does not work.

What are the symptoms? Not sure what difference does anonymous namespace make.

Attempt 1:

Linker error with just dropping the static in front of these. Because we now have duplicate template instantiations between RISCV and AMDGPU and they conflict.

Attempt 2:
If I also put namespace {} surrounding the specializations (to address the linker errors in 1), I get error "class template specialization not in a namespace enclosing" because since InsnBitWidth is in namesapce llvm::MCD, its specializations are also expected to be in that same namespace. That means the conflict between AMDGPU and RISCV is impossible to prevent.

Current PR:
Move InsnBitWidth to anonymous namespace, and then its specializations can also be in anonymous namespace and thus private to each target, avoiding any linker errors.

[jurahul]

Its weird though that these are just regular pre-commit checks that are failing once I committed the change. Why did my PR pass the pre-commit checks which run these same checks?

[jurahul]

Note that @MaskRay 's earlier fix a7d1a65 sort of works, because it just changed RISCV and not AMDGPU (so does not run into linker errors). So, it's a good stopgap and reduces the urgency of this (more correct) fix.

[llvmbot]

@llvm/pr-subscribers-tablegen

@llvm/pr-subscribers-backend-risc-v

Author: Rahul Joshi (jurahul)

Changes

Move InsnBitWidth template into anonymous namespace in the generated code and move template specialization of InsnBitWidth to anonymous namespace as well, and drop static for them. This makes InsnBitWidth completely private to each target and fixes the "explicit specialization cannot have a storage class" warning as well as any potential linker errors if InsnBitWidth is kept in the llvm::MCD namespace.

---

Full diff: https://github.com/llvm/llvm-project/pull/156375.diff

4 Files Affected:

• (modified) llvm/include/llvm/MC/MCDecoder.h (-7)
• (modified) llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp (+6-6)
• (modified) llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp (+5-3)
• (modified) llvm/utils/TableGen/DecoderEmitter.cpp (+8)

diff --git a/llvm/include/llvm/MC/MCDecoder.h b/llvm/include/llvm/MC/MCDecoder.h
index 459c8a6a5ea34..87df6c10d8bb2 100644
--- a/llvm/include/llvm/MC/MCDecoder.h
+++ b/llvm/include/llvm/MC/MCDecoder.h
@@ -72,13 +72,6 @@ insertBits(IntType &field, IntType bits, unsigned startBit, unsigned numBits) {
   field |= bits << startBit;
 }
 
-// InsnBitWidth is essentially a type trait used by the decoder emitter to query
-// the supported bitwidth for a given type. But default, the value is 0, making
-// it an invalid type for use as `InsnType` when instantiating the decoder.
-// Individual targets are expected to provide specializations for these based
-// on their usage.
-template <typename T> static constexpr uint32_t InsnBitWidth = 0;
-
 } // namespace llvm::MCD
 
 #endif // LLVM_MC_MCDECODER_H
diff --git a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
index 80d194afa926b..bb9f811683255 100644
--- a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
+++ b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
@@ -447,13 +447,13 @@ static DecodeStatus decodeVersionImm(MCInst &Inst, unsigned Imm,
 
 #include "AMDGPUGenDisassemblerTables.inc"
 
+namespace {
 // Define bitwidths for various types used to instantiate the decoder.
-template <> static constexpr uint32_t llvm::MCD::InsnBitWidth<uint32_t> = 32;
-template <> static constexpr uint32_t llvm::MCD::InsnBitWidth<uint64_t> = 64;
-template <>
-static constexpr uint32_t llvm::MCD::InsnBitWidth<std::bitset<96>> = 96;
-template <>
-static constexpr uint32_t llvm::MCD::InsnBitWidth<std::bitset<128>> = 128;
+template <> constexpr uint32_t InsnBitWidth<uint32_t> = 32;
+template <> constexpr uint32_t InsnBitWidth<uint64_t> = 64;
+template <> constexpr uint32_t InsnBitWidth<std::bitset<96>> = 96;
+template <> constexpr uint32_t InsnBitWidth<std::bitset<128>> = 128;
+} // namespace
 
 //===----------------------------------------------------------------------===//
 //
diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index 395672fe5b68b..b1b7ea5246fda 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -701,11 +701,13 @@ static constexpr DecoderListEntry DecoderList32[]{
     {DecoderTableZdinxRV32Only32, {}, "RV32-only Zdinx (Double in Integer)"},
 };
 
+namespace {
 // Define bitwidths for various types used to instantiate the decoder.
-template <> constexpr uint32_t llvm::MCD::InsnBitWidth<uint16_t> = 16;
-template <> constexpr uint32_t llvm::MCD::InsnBitWidth<uint32_t> = 32;
+template <> constexpr uint32_t InsnBitWidth<uint16_t> = 16;
+template <> constexpr uint32_t InsnBitWidth<uint32_t> = 32;
 // Use uint64_t to represent 48 bit instructions.
-template <> constexpr uint32_t llvm::MCD::InsnBitWidth<uint64_t> = 48;
+template <> constexpr uint32_t InsnBitWidth<uint64_t> = 48;
+} // namespace
 
 DecodeStatus RISCVDisassembler::getInstruction32(MCInst &MI, uint64_t &Size,
                                                  ArrayRef<uint8_t> Bytes,
diff --git a/llvm/utils/TableGen/DecoderEmitter.cpp b/llvm/utils/TableGen/DecoderEmitter.cpp
index 354c2a788d5b1..6d28fa98ba9a3 100644
--- a/llvm/utils/TableGen/DecoderEmitter.cpp
+++ b/llvm/utils/TableGen/DecoderEmitter.cpp
@@ -2479,6 +2479,14 @@ void DecoderEmitter::run(raw_ostream &o) const {
 #include <assert.h>
 
 namespace {
+
+// InsnBitWidth is essentially a type trait used by the decoder emitter to query
+// the supported bitwidth for a given type. But default, the value is 0, making
+// it an invalid type for use as `InsnType` when instantiating the decoder.
+// Individual targets are expected to provide specializations for these based
+// on their usage.
+template <typename T> static constexpr uint32_t InsnBitWidth = 0;
+
 )";
 
   // Do extra bookkeeping for variable-length encodings.

[s-barannikov]

Linker error with just dropping the static in front of these. Because we now have duplicate template instantiations between RISCV and AMDGPU and they conflict.

This is confusing. Why does it warn about static being redundant while it clearly isn't?

[jurahul]

Right, the static seems illegal per the C++ standard to have a storage class for template specialization. And the other rule is that template specializations need to belong to the same namespace as the template being specialized. These 2 rules seem to interact here to result in a linker error if we attempt to remove the static from the specializations in both AMDGPU and RISCV while keeping them in llvm::MCD namespace. My solution to just move them to anonymous namespace (both the template and its specializations).

[jurahul]

Note, @kazutakahirata added one more fix by making them inline, but I suspect its susceptible to the same issue. Maybe not a linker error, but since we have 2 different specializations for uint64_t with different values (64 vs 48). wrong ones being picked up potentially. In practice likely not right now, but I still feel my fix to avoid any sort of visibility of these specializations across targets through the llvm::MCD namespace is a more robust fix.

Apologies for the churn over the long weekend :(

[kazutakahirata]

@jurahul Could you rebase this PR? I think it's the right thing to hide these constants in anonymous namespaces, but I just want to build this myself. Thanks!

[jurahul]

@jurahul Could you rebase this PR? I think it's the right thing to hide these constants in anonymous namespaces, but I just want to build this myself. Thanks!

Done

[kazutakahirata]

LGTM. Thank you for rebasing this PR! I've confirmed the build is OK with this patch.