Address reviewer feedback.

Author: perlfu

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

@@ -55,7 +55,7 @@ AMDGPUDisassembler::AMDGPUDisassembler(const MCSubtargetInfo &STI,
     report_fatal_error("Disassembly not yet supported for subtarget");
 
   for (auto [Symbol, Code] : AMDGPU::UCVersion::getGFXVersions())
-    UCVersionSymbols.push_back(createConstantSymbolExpr(Symbol, Code));
+    createConstantSymbolExpr(Symbol, Code);
 
   UCVersionW64Expr = createConstantSymbolExpr("UC_VERSION_W64_BIT", 0x2000);
   UCVersionW32Expr = createConstantSymbolExpr("UC_VERSION_W32_BIT", 0x4000);
@@ -1755,25 +1755,18 @@ MCOperand AMDGPUDisassembler::decodeVersionImm(unsigned Imm) const {
   if (Encoding::encode(Version, W64, W32, MDP) != Imm)
     return MCOperand::createImm(Imm);
 
-  // Locate UC_VERSION symbol matching Version.
+  const auto &Versions = AMDGPU::UCVersion::getGFXVersions();
+  auto I = find_if(Versions,
+                   [Version = Version](const AMDGPU::UCVersion::GFXVersion &V) {
+                     return V.Code == Version;
+                   });
   MCContext &Ctx = getContext();
-  const MCExpr *E = nullptr;
-  for (auto *VersionSym : UCVersionSymbols) {
-    int64_t Val;
-    if (!VersionSym->evaluateAsAbsolute(Val))
-      continue;
-    if (Val != (int64_t)Version)
-      continue;
-    auto *Sym = Ctx.getOrCreateSymbol(VersionSym->getSymbol().getName());
-    E = MCSymbolRefExpr::create(Sym, Ctx);
-    break;
-  }
-
-  // Default to constant value if not found.
-  if (!E)
+  const MCExpr *E;
+  if (I == Versions.end())
     E = MCConstantExpr::create(Version, Ctx);
+  else
+    E = MCSymbolRefExpr::create(Ctx.getOrCreateSymbol(I->Symbol), Ctx);
 
-  // Apply bits.
   if (W64)
     E = MCBinaryExpr::createOr(E, UCVersionW64Expr, Ctx);
   if (W32)
@@ -2369,17 +2362,17 @@ Expected<bool> AMDGPUDisassembler::onSymbolStart(SymbolInfoTy &Symbol,
   return false;
 }
 
-const MCSymbolRefExpr *
-AMDGPUDisassembler::createConstantSymbolExpr(StringRef Id, int64_t Val) {
+const MCExpr *AMDGPUDisassembler::createConstantSymbolExpr(StringRef Id,
+                                                           int64_t Val) {
   MCContext &Ctx = getContext();
   MCSymbol *Sym = Ctx.getOrCreateSymbol(Id);
-  // Note: only set value to Val on a new symbol.
-  // Existing symbol may potentially have a different value to the one
-  // requested, which allows for user redefinition of symbols.
-  if (!Sym->isVariable()) {
-    Sym->setRedefinable(true);
+  // Note: only set value to Val on a new symbol in case an dissassembler
+  // has already been initialized in this context.
+  [[maybe_unused]] int64_t Res = ~Val;
+  assert(!Sym->isVariable() ||
+         (Sym->getVariableValue()->evaluateAsAbsolute(Res) && Res == Val));
+  if (!Sym->isVariable())
     Sym->setVariableValue(MCConstantExpr::create(Val, Ctx));
-  }
   return MCSymbolRefExpr::create(Sym, Ctx);
 }
 

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h

@@ -30,7 +30,6 @@ class MCAsmInfo;
 class MCInst;
 class MCOperand;
 class MCSubtargetInfo;
-class MCSymbolRefExpr;
 class Twine;
 
 // Exposes an interface expected by autogenerated code in
@@ -106,9 +105,8 @@ class AMDGPUDisassembler : public MCDisassembler {
   const MCExpr *UCVersionW64Expr;
   const MCExpr *UCVersionW32Expr;
   const MCExpr *UCVersionMDPExpr;
-  SmallVector<const MCSymbolRefExpr *> UCVersionSymbols;
 
-  const MCSymbolRefExpr *createConstantSymbolExpr(StringRef Id, int64_t Val);
+  const MCExpr *createConstantSymbolExpr(StringRef Id, int64_t Val);
 
 public:
   AMDGPUDisassembler(const MCSubtargetInfo &STI, MCContext &Ctx,

llvm/test/MC/AMDGPU/custom-uc-version.s

@@ -132,71 +132,3 @@ TEST(AMDGPUDisassembler, MultiDisassembler) {
   StrBuffer[InsnStr.size()] = '\0';
   EXPECT_EQ(StringRef(StrBuffer), "\ts_version UC_VERSION_GFX10");
 }
-
-// Test UC_VERSION symbols can be overriden.
-TEST(AMDGPUDisassembler, UCVersionOverride) {
-  LLVMInitializeAMDGPUTargetInfo();
-  LLVMInitializeAMDGPUTargetMC();
-  LLVMInitializeAMDGPUDisassembler();
-
-  std::string Error;
-  const Target *TheTarget = TargetRegistry::lookupTarget(TripleName, Error);
-
-  // Skip test if AMDGPU not built.
-  if (!TheTarget)
-    GTEST_SKIP();
-
-  std::unique_ptr<MCRegisterInfo> MRI(TheTarget->createMCRegInfo(TripleName));
-  std::unique_ptr<MCAsmInfo> MAI(
-      TheTarget->createMCAsmInfo(*MRI, TripleName, MCTargetOptions()));
-  std::unique_ptr<const MCInstrInfo> MII(TheTarget->createMCInstrInfo());
-  std::unique_ptr<MCSubtargetInfo> STI(
-      TheTarget->createMCSubtargetInfo(TripleName, CPUName, ""));
-  auto Ctx = std::make_unique<MCContext>(Triple(TripleName), MAI.get(),
-                                         MRI.get(), STI.get());
-
-  // Define custom UC_VERSION before initializing disassembler.
-  const uint8_t UC_VERSION_GFX10_DEFAULT = 0x04;
-  const uint8_t UC_VERSION_GFX10_NEW = 0x99;
-  auto Sym = Ctx->getOrCreateSymbol("UC_VERSION_GFX10");
-  Sym->setVariableValue(MCConstantExpr::create(UC_VERSION_GFX10_NEW, *Ctx));
-
-  int AsmPrinterVariant = MAI->getAssemblerDialect();
-  std::unique_ptr<MCInstPrinter> IP(TheTarget->createMCInstPrinter(
-      Triple(TripleName), AsmPrinterVariant, *MAI, *MII, *MRI));
-
-  std::unique_ptr<MCDisassembler> DisAsm(
-      TheTarget->createMCDisassembler(*STI, *Ctx));
-
-  SmallVector<char, 64> InsnStr, AnnoStr;
-  raw_svector_ostream OS(InsnStr);
-  raw_svector_ostream Annotations(AnnoStr);
-  formatted_raw_ostream FormattedOS(OS);
-
-  char StrBuffer[128];
-
-  // Decode S_VERSION instruction with original or custom version.
-  uint8_t Versions[] = {UC_VERSION_GFX10_DEFAULT, UC_VERSION_GFX10_NEW};
-  for (uint8_t Version : Versions) {
-    uint8_t Bytes[] = {Version, 0x00, 0x80, 0xb0};
-    size_t InstSize = 0U;
-    MCInst Inst;
-
-    AnnoStr.clear();
-    InsnStr.clear();
-    MCDisassembler::DecodeStatus Status =
-        DisAsm->getInstruction(Inst, InstSize, Bytes, 0, Annotations);
-    ASSERT_TRUE(Status == MCDisassembler::Success);
-    EXPECT_EQ(InstSize, 4U);
-
-    IP->printInst(&Inst, 0, Annotations.str(), *STI, FormattedOS);
-    ASSERT_TRUE(InsnStr.size() < (sizeof(StrBuffer) - 1));
-    std::memcpy(StrBuffer, InsnStr.data(), InsnStr.size());
-    StrBuffer[InsnStr.size()] = '\0';
-
-    if (Version == UC_VERSION_GFX10_DEFAULT)
-      EXPECT_EQ(StringRef(StrBuffer), "\ts_version 4");
-    else
-      EXPECT_EQ(StringRef(StrBuffer), "\ts_version UC_VERSION_GFX10");
-  }
-}