Skip to content

Conversation

jurahul
Copy link
Contributor

@jurahul jurahul commented Aug 21, 2025

  • use llvm::endian::read<> to read bit/little endian.
  • Range check against size of the lookup tables instead of hardcoded numbers.
  • Make lookup tables constexpr.
  • Drop {} for single-statement if-else.

@jurahul jurahul force-pushed the nfc_sparc_disasm_cleanup branch 2 times, most recently from f39329a to 3477934 Compare August 21, 2025 20:22
- use llvm::endian::read<> to read bit/little endian.
- Range check against size of the lookup tables instead of hardcoded
  numbers.
- Make lookup tables constexpr.
- Drop {} for single-statement if-else.
@jurahul jurahul force-pushed the nfc_sparc_disasm_cleanup branch from 3477934 to 98e993e Compare August 21, 2025 21:23
@jurahul jurahul marked this pull request as ready for review August 21, 2025 22:07
@llvmbot
Copy link
Member

llvmbot commented Aug 21, 2025

@llvm/pr-subscribers-backend-sparc

Author: Rahul Joshi (jurahul)

Changes
  • use llvm::endian::read<> to read bit/little endian.
  • Range check against size of the lookup tables instead of hardcoded numbers.
  • Make lookup tables constexpr.
  • Drop {} for single-statement if-else.

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

1 Files Affected:

  • (modified) llvm/lib/Target/Sparc/Disassembler/SparcDisassembler.cpp (+33-35)
diff --git a/llvm/lib/Target/Sparc/Disassembler/SparcDisassembler.cpp b/llvm/lib/Target/Sparc/Disassembler/SparcDisassembler.cpp
index fab94fb4d40ca..e236dd4c9598d 100644
--- a/llvm/lib/Target/Sparc/Disassembler/SparcDisassembler.cpp
+++ b/llvm/lib/Target/Sparc/Disassembler/SparcDisassembler.cpp
@@ -19,6 +19,7 @@
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/Endian.h"
 
 using namespace llvm;
 
@@ -58,7 +59,8 @@ LLVMInitializeSparcDisassembler() {
                                          createSparcDisassembler);
 }
 
-static const unsigned IntRegDecoderTable[] = {
+// clang-format off
+static constexpr unsigned IntRegDecoderTable[] = {
   SP::G0,  SP::G1,  SP::G2,  SP::G3,
   SP::G4,  SP::G5,  SP::G6,  SP::G7,
   SP::O0,  SP::O1,  SP::O2,  SP::O3,
@@ -68,7 +70,7 @@ static const unsigned IntRegDecoderTable[] = {
   SP::I0,  SP::I1,  SP::I2,  SP::I3,
   SP::I4,  SP::I5,  SP::I6,  SP::I7 };
 
-static const unsigned FPRegDecoderTable[] = {
+static constexpr unsigned FPRegDecoderTable[] = {
   SP::F0,   SP::F1,   SP::F2,   SP::F3,
   SP::F4,   SP::F5,   SP::F6,   SP::F7,
   SP::F8,   SP::F9,   SP::F10,  SP::F11,
@@ -78,7 +80,7 @@ static const unsigned FPRegDecoderTable[] = {
   SP::F24,  SP::F25,  SP::F26,  SP::F27,
   SP::F28,  SP::F29,  SP::F30,  SP::F31 };
 
-static const unsigned DFPRegDecoderTable[] = {
+static constexpr unsigned DFPRegDecoderTable[] = {
   SP::D0,   SP::D16,  SP::D1,   SP::D17,
   SP::D2,   SP::D18,  SP::D3,   SP::D19,
   SP::D4,   SP::D20,  SP::D5,   SP::D21,
@@ -88,7 +90,7 @@ static const unsigned DFPRegDecoderTable[] = {
   SP::D12,  SP::D28,  SP::D13,  SP::D29,
   SP::D14,  SP::D30,  SP::D15,  SP::D31 };
 
-static const unsigned QFPRegDecoderTable[] = {
+static constexpr unsigned QFPRegDecoderTable[] = {
   SP::Q0,  SP::Q8,   ~0U,  ~0U,
   SP::Q1,  SP::Q9,   ~0U,  ~0U,
   SP::Q2,  SP::Q10,  ~0U,  ~0U,
@@ -98,29 +100,29 @@ static const unsigned QFPRegDecoderTable[] = {
   SP::Q6,  SP::Q14,  ~0U,  ~0U,
   SP::Q7,  SP::Q15,  ~0U,  ~0U } ;
 
-static const unsigned FCCRegDecoderTable[] = {
+static constexpr unsigned FCCRegDecoderTable[] = {
   SP::FCC0, SP::FCC1, SP::FCC2, SP::FCC3 };
 
-static const unsigned ASRRegDecoderTable[] = {
+static constexpr unsigned ASRRegDecoderTable[] = {
     SP::Y,     SP::ASR1,  SP::ASR2,  SP::ASR3,  SP::ASR4,  SP::ASR5,  SP::ASR6,
     SP::ASR7,  SP::ASR8,  SP::ASR9,  SP::ASR10, SP::ASR11, SP::ASR12, SP::ASR13,
     SP::ASR14, SP::ASR15, SP::ASR16, SP::ASR17, SP::ASR18, SP::ASR19, SP::ASR20,
     SP::ASR21, SP::ASR22, SP::ASR23, SP::ASR24, SP::ASR25, SP::ASR26, SP::ASR27,
     SP::ASR28, SP::ASR29, SP::ASR30, SP::ASR31};
 
-static const unsigned PRRegDecoderTable[] = {
+static constexpr unsigned PRRegDecoderTable[] = {
     SP::TPC,     SP::TNPC,       SP::TSTATE,   SP::TT,       SP::TICK,
     SP::TBA,     SP::PSTATE,     SP::TL,       SP::PIL,      SP::CWP,
     SP::CANSAVE, SP::CANRESTORE, SP::CLEANWIN, SP::OTHERWIN, SP::WSTATE};
 
-static const uint16_t IntPairDecoderTable[] = {
+static constexpr uint16_t IntPairDecoderTable[] = {
   SP::G0_G1, SP::G2_G3, SP::G4_G5, SP::G6_G7,
   SP::O0_O1, SP::O2_O3, SP::O4_O5, SP::O6_O7,
   SP::L0_L1, SP::L2_L3, SP::L4_L5, SP::L6_L7,
   SP::I0_I1, SP::I2_I3, SP::I4_I5, SP::I6_I7,
 };
 
-static const unsigned CPRegDecoderTable[] = {
+static constexpr unsigned CPRegDecoderTable[] = {
   SP::C0,  SP::C1,  SP::C2,  SP::C3,
   SP::C4,  SP::C5,  SP::C6,  SP::C7,
   SP::C8,  SP::C9,  SP::C10, SP::C11,
@@ -131,18 +133,18 @@ static const unsigned CPRegDecoderTable[] = {
   SP::C28, SP::C29, SP::C30, SP::C31
 };
 
-
-static const uint16_t CPPairDecoderTable[] = {
+static constexpr uint16_t CPPairDecoderTable[] = {
   SP::C0_C1,   SP::C2_C3,   SP::C4_C5,   SP::C6_C7,
   SP::C8_C9,   SP::C10_C11, SP::C12_C13, SP::C14_C15,
   SP::C16_C17, SP::C18_C19, SP::C20_C21, SP::C22_C23,
   SP::C24_C25, SP::C26_C27, SP::C28_C29, SP::C30_C31
 };
+// clang-format on
 
 static DecodeStatus DecodeIntRegsRegisterClass(MCInst &Inst, unsigned RegNo,
                                                uint64_t Address,
                                                const MCDisassembler *Decoder) {
-  if (RegNo > 31)
+  if (RegNo >= std::size(IntRegDecoderTable))
     return MCDisassembler::Fail;
   unsigned Reg = IntRegDecoderTable[RegNo];
   Inst.addOperand(MCOperand::createReg(Reg));
@@ -166,7 +168,7 @@ static DecodeStatus DecodePointerLikeRegClass0(MCInst &Inst, unsigned RegNo,
 static DecodeStatus DecodeFPRegsRegisterClass(MCInst &Inst, unsigned RegNo,
                                               uint64_t Address,
                                               const MCDisassembler *Decoder) {
-  if (RegNo > 31)
+  if (RegNo >= std::size(FPRegDecoderTable))
     return MCDisassembler::Fail;
   unsigned Reg = FPRegDecoderTable[RegNo];
   Inst.addOperand(MCOperand::createReg(Reg));
@@ -176,7 +178,7 @@ static DecodeStatus DecodeFPRegsRegisterClass(MCInst &Inst, unsigned RegNo,
 static DecodeStatus DecodeDFPRegsRegisterClass(MCInst &Inst, unsigned RegNo,
                                                uint64_t Address,
                                                const MCDisassembler *Decoder) {
-  if (RegNo > 31)
+  if (RegNo >= std::size(DFPRegDecoderTable))
     return MCDisassembler::Fail;
   unsigned Reg = DFPRegDecoderTable[RegNo];
   Inst.addOperand(MCOperand::createReg(Reg));
@@ -186,7 +188,7 @@ static DecodeStatus DecodeDFPRegsRegisterClass(MCInst &Inst, unsigned RegNo,
 static DecodeStatus DecodeQFPRegsRegisterClass(MCInst &Inst, unsigned RegNo,
                                                uint64_t Address,
                                                const MCDisassembler *Decoder) {
-  if (RegNo > 31)
+  if (RegNo >= std::size(QFPRegDecoderTable))
     return MCDisassembler::Fail;
 
   unsigned Reg = QFPRegDecoderTable[RegNo];
@@ -199,7 +201,7 @@ static DecodeStatus DecodeQFPRegsRegisterClass(MCInst &Inst, unsigned RegNo,
 static DecodeStatus
 DecodeCoprocRegsRegisterClass(MCInst &Inst, unsigned RegNo, uint64_t Address,
                               const MCDisassembler *Decoder) {
-  if (RegNo > 31)
+  if (RegNo >= std::size(CPRegDecoderTable))
     return MCDisassembler::Fail;
   unsigned Reg = CPRegDecoderTable[RegNo];
   Inst.addOperand(MCOperand::createReg(Reg));
@@ -209,7 +211,7 @@ DecodeCoprocRegsRegisterClass(MCInst &Inst, unsigned RegNo, uint64_t Address,
 static DecodeStatus DecodeFCCRegsRegisterClass(MCInst &Inst, unsigned RegNo,
                                                uint64_t Address,
                                                const MCDisassembler *Decoder) {
-  if (RegNo > 3)
+  if (RegNo >= std::size(FCCRegDecoderTable))
     return MCDisassembler::Fail;
   Inst.addOperand(MCOperand::createReg(FCCRegDecoderTable[RegNo]));
   return MCDisassembler::Success;
@@ -218,7 +220,7 @@ static DecodeStatus DecodeFCCRegsRegisterClass(MCInst &Inst, unsigned RegNo,
 static DecodeStatus DecodeASRRegsRegisterClass(MCInst &Inst, unsigned RegNo,
                                                uint64_t Address,
                                                const MCDisassembler *Decoder) {
-  if (RegNo > 31)
+  if (RegNo >= std::size(ASRRegDecoderTable))
     return MCDisassembler::Fail;
   Inst.addOperand(MCOperand::createReg(ASRRegDecoderTable[RegNo]));
   return MCDisassembler::Success;
@@ -238,13 +240,14 @@ static DecodeStatus DecodeIntPairRegisterClass(MCInst &Inst, unsigned RegNo,
                                                const MCDisassembler *Decoder) {
   DecodeStatus S = MCDisassembler::Success;
 
-  if (RegNo > 31)
-    return MCDisassembler::Fail;
-
   if ((RegNo & 1))
     S = MCDisassembler::SoftFail;
 
-  unsigned RegisterPair = IntPairDecoderTable[RegNo/2];
+  RegNo = RegNo / 2;
+  if (RegNo >= std::size(IntPairDecoderTable))
+    return MCDisassembler::Fail;
+
+  unsigned RegisterPair = IntPairDecoderTable[RegNo];
   Inst.addOperand(MCOperand::createReg(RegisterPair));
   return S;
 }
@@ -252,10 +255,11 @@ static DecodeStatus DecodeIntPairRegisterClass(MCInst &Inst, unsigned RegNo,
 static DecodeStatus
 DecodeCoprocPairRegisterClass(MCInst &Inst, unsigned RegNo, uint64_t Address,
                               const MCDisassembler *Decoder) {
-  if (RegNo > 31)
+  RegNo = RegNo / 2;
+  if (RegNo >= std::size(CPPairDecoderTable))
     return MCDisassembler::Fail;
 
-  unsigned RegisterPair = CPPairDecoderTable[RegNo/2];
+  unsigned RegisterPair = CPPairDecoderTable[RegNo];
   Inst.addOperand(MCOperand::createReg(RegisterPair));
   return MCDisassembler::Success;
 }
@@ -283,12 +287,8 @@ static DecodeStatus readInstruction32(ArrayRef<uint8_t> Bytes, uint64_t Address,
   }
 
   Size = 4;
-  Insn = IsLittleEndian
-             ? (Bytes[0] << 0) | (Bytes[1] << 8) | (Bytes[2] << 16) |
-                   (Bytes[3] << 24)
-             : (Bytes[3] << 0) | (Bytes[2] << 8) | (Bytes[1] << 16) |
-                   (Bytes[0] << 24);
-
+  Insn = support::endian::read<uint32_t>(
+      Bytes.data(), IsLittleEndian ? endianness::little : endianness::big);
   return MCDisassembler::Success;
 }
 
@@ -306,13 +306,11 @@ DecodeStatus SparcDisassembler::getInstruction(MCInst &Instr, uint64_t &Size,
   // Calling the auto-generated decoder function.
 
   if (STI.hasFeature(Sparc::FeatureV9))
-  {
-    Result = decodeInstruction(DecoderTableSparcV932, Instr, Insn, Address, this, STI);
-  }
+    Result = decodeInstruction(DecoderTableSparcV932, Instr, Insn, Address,
+                               this, STI);
   else
-  {
     Result = decodeInstruction(DecoderTableSparcV832, Instr, Insn, Address, this, STI);
-  }
+
   if (Result != MCDisassembler::Fail)
     return Result;
 

@jurahul jurahul merged commit 6372651 into llvm:main Aug 22, 2025
12 checks passed
@jurahul jurahul deleted the nfc_sparc_disasm_cleanup branch August 22, 2025 14:22
@llvm-ci
Copy link
Collaborator

llvm-ci commented Aug 22, 2025

LLVM Buildbot has detected a new failure on builder clang-armv8-quick running on linaro-clang-armv8-quick while building llvm at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/20575

Here is the relevant piece of the build log for the reference
Step 5 (ninja check 1) failure: stage 1 checked (failure)
******************** TEST 'Clangd Unit Tests :: ./ClangdTests/61/332' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/tools/clang/tools/extra/clangd/unittests/./ClangdTests-Clangd Unit Tests-3060018-61-332.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=332 GTEST_SHARD_INDEX=61 /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/tools/clang/tools/extra/clangd/unittests/./ClangdTests
--

Script:
--
/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/tools/clang/tools/extra/clangd/unittests/./ClangdTests --gtest_filter=ClangdThreadingTest.StressTest
--
File version went from null to null
File version went from null to null
File version went from null to null
ASTWorker skipping Update for /clangd-test/Foo1.cpp
File version went from null to null
File version went from null to null
ASTWorker building file /clangd-test/Foo0.cpp version null with command 
[/clangd-test]
clang -ffreestanding /clangd-test/Foo0.cpp
ASTWorker building file /clangd-test/Foo0.cpp version null with command 
[/clangd-test]
clang -ffreestanding /clangd-test/Foo0.cpp
ASTWorker skipping Update for /clangd-test/Foo0.cpp
ASTWorker skipping Update for /clangd-test/Foo2.cpp
File version went from null to null
File version went from null to null
File version went from null to null
File version went from null to null
File version went from null to null
File version went from null to null
File version went from null to null
File version went from null to null
File version went from null to null
File version went from null to null
File version went from null to null
File version went from null to null
ASTWorker skipping Update for /clangd-test/Foo4.cpp
ASTWorker skipping Update for /clangd-test/Foo4.cpp
ASTWorker skipping Update for /clangd-test/Foo4.cpp
ASTWorker building file /clangd-test/Foo2.cpp version null with command 
[/clangd-test]
clang -ffreestanding /clangd-test/Foo2.cpp
ASTWorker building file /clangd-test/Foo4.cpp version null with command 
[/clangd-test]
clang -ffreestanding /clangd-test/Foo4.cpp
Driver produced command: cc1 -cc1 -triple armv8a-unknown-linux-gnueabihf -fsyntax-only -disable-free -clear-ast-before-backend -main-file-name Foo0.cpp -mrelocation-model pic -pic-level 2 -pic-is-pie -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -ffreestanding -target-cpu generic -target-feature +read-tp-tpidruro -target-feature +vfp2 -target-feature +vfp2sp -target-feature +vfp3 -target-feature +vfp3d16 -target-feature +vfp3d16sp -target-feature +vfp3sp -target-feature +fp16 -target-feature +vfp4 -target-feature +vfp4d16 -target-feature +vfp4d16sp -target-feature +vfp4sp -target-feature +fp-armv8 -target-feature +fp-armv8d16 -target-feature +fp-armv8d16sp -target-feature +fp-armv8sp -target-feature -fullfp16 -target-feature +fp64 -target-feature +d32 -target-feature +sha2 -target-feature +aes -target-feature -fp16fml -target-feature +neon -target-abi aapcs-linux -mfloat-abi hard -debugger-tuning=gdb -fdebug-compilation-dir=/clangd-test -fcoverage-compilation-dir=/clangd-test -resource-dir lib/clang/22 -internal-isystem lib/clang/22/include -internal-isystem /usr/local/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -fdeprecated-macro -ferror-limit 19 -fno-signed-char -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -no-round-trip-args -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -x c++ /clangd-test/Foo0.cpp
Driver produced command: cc1 -cc1 -triple armv8a-unknown-linux-gnueabihf -fsyntax-only -disable-free -clear-ast-before-backend -main-file-name Foo2.cpp -mrelocation-model pic -pic-level 2 -pic-is-pie -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -ffreestanding -target-cpu generic -target-feature +read-tp-tpidruro -target-feature +vfp2 -target-feature +vfp2sp -target-feature +vfp3 -target-feature +vfp3d16 -target-feature +vfp3d16sp -target-feature +vfp3sp -target-feature +fp16 -target-feature +vfp4 -target-feature +vfp4d16 -target-feature +vfp4d16sp -target-feature +vfp4sp -target-feature +fp-armv8 -target-feature +fp-armv8d16 -target-feature +fp-armv8d16sp -target-feature +fp-armv8sp -target-feature -fullfp16 -target-feature +fp64 -target-feature +d32 -target-feature +sha2 -target-feature +aes -target-feature -fp16fml -target-feature +neon -target-abi aapcs-linux -mfloat-abi hard -debugger-tuning=gdb -fdebug-compilation-dir=/clangd-test -fcoverage-compilation-dir=/clangd-test -resource-dir lib/clang/22 -internal-isystem lib/clang/22/include -internal-isystem /usr/local/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -fdeprecated-macro -ferror-limit 19 -fno-signed-char -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -no-round-trip-args -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -x c++ /clangd-test/Foo2.cpp
Driver produced command: cc1 -cc1 -triple armv8a-unknown-linux-gnueabihf -fsyntax-only -disable-free -clear-ast-before-backend -main-file-name Foo0.cpp -mrelocation-model pic -pic-level 2 -pic-is-pie -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -ffreestanding -target-cpu generic -target-feature +read-tp-tpidruro -target-feature +vfp2 -target-feature +vfp2sp -target-feature +vfp3 -target-feature +vfp3d16 -target-feature +vfp3d16sp -target-feature +vfp3sp -target-feature +fp16 -target-feature +vfp4 -target-feature +vfp4d16 -target-feature +vfp4d16sp -target-feature +vfp4sp -target-feature +fp-armv8 -target-feature +fp-armv8d16 -target-feature +fp-armv8d16sp -target-feature +fp-armv8sp -target-feature -fullfp16 -target-feature +fp64 -target-feature +d32 -target-feature +sha2 -target-feature +aes -target-feature -fp16fml -target-feature +neon -target-abi aapcs-linux -mfloat-abi hard -debugger-tuning=gdb -fdebug-compilation-dir=/clangd-test -fcoverage-compilation-dir=/clangd-test -resource-dir lib/clang/22 -internal-isystem lib/clang/22/include -internal-isystem /usr/local/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -fdeprecated-macro -ferror-limit 19 -fno-signed-char -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -no-round-trip-args -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -x c++ /clangd-test/Foo0.cpp
Driver produced command: cc1 -cc1 -triple armv8a-unknown-linux-gnueabihf -fsyntax-only -disable-free -clear-ast-before-backend -main-file-name Foo4.cpp -mrelocation-model pic -pic-level 2 -pic-is-pie -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -ffreestanding -target-cpu generic -target-feature +read-tp-tpidruro -target-feature +vfp2 -target-feature +vfp2sp -target-feature +vfp3 -target-feature +vfp3d16 -target-feature +vfp3d16sp -target-feature +vfp3sp -target-feature +fp16 -target-feature +vfp4 -target-feature +vfp4d16 -target-feature +vfp4d16sp -target-feature +vfp4sp -target-feature +fp-armv8 -target-feature +fp-armv8d16 -target-feature +fp-armv8d16sp -target-feature +fp-armv8sp -target-feature -fullfp16 -target-feature +fp64 -target-feature +d32 -target-feature +sha2 -target-feature +aes -target-feature -fp16fml -target-feature +neon -target-abi aapcs-linux -mfloat-abi hard -debugger-tuning=gdb -fdebug-compilation-dir=/clangd-test -fcoverage-compilation-dir=/clangd-test -resource-dir lib/clang/22 -internal-isystem lib/clang/22/include -internal-isystem /usr/local/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -fdeprecated-macro -ferror-limit 19 -fno-signed-char -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -no-round-trip-args -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -x c++ /clangd-test/Foo4.cpp
ASTWorker building file /clangd-test/Foo0.cpp version null with command 
...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants