[AMDGPU][AsmParser][NFC] Give isImmLiteral() a better name. #153395

kosarev · 2025-08-13T11:31:02Z

We used to call literals what are not inline immediates. We seem already call them plain immediates in the code, so maybe use that.

Also rename ImmTyNone, which is confusing as well, since they are not really immediates of no kind.

Simplify related code while there.

We used to call literals what are not inline immediates. We seem already call them plain immediates in the code, so maybe use that. Also rename ImmTyNone, which is confusing as well, since they are not really immediates of no kind. Simplify related code while there.

llvmbot · 2025-08-13T11:31:44Z

@llvm/pr-subscribers-backend-amdgpu

Author: Ivan Kosarev (kosarev)

Changes

We used to call literals what are not inline immediates. We seem already call them plain immediates in the code, so maybe use that.

Also rename ImmTyNone, which is confusing as well, since they are not really immediates of no kind.

Simplify related code while there.

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

2 Files Affected:

(modified) llvm/lib/Target/AMDGPU/AMDGPUInstructions.td (+1-1)
(modified) llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp (+26-28)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td b/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
index 511fc6967da31..e746b7d602423 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
+++ b/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
@@ -134,7 +134,7 @@ class CustomOperand<ValueType type, bit optional = 0, string name = NAME>
 class ImmOperand<ValueType type, string name = NAME, bit optional = 0,
                  string printer = "print"#name>
     : CustomOperand<type, optional, name> {
-  let ImmTy = "ImmTyNone";
+  let ImmTy = "ImmTyPlain";
   let ParserMethod = "";
   let PrintMethod = printer;
 }
diff --git a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
index 0184075c2c909..29329a8293315 100644
--- a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
+++ b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
@@ -114,7 +114,7 @@ class AMDGPUOperand : public MCParsedAsmOperand {
   };
 
   enum ImmTy {
-    ImmTyNone,
+    ImmTyPlain,
     ImmTyGDS,
     ImmTyLDS,
     ImmTyOffen,
@@ -408,11 +408,9 @@ class AMDGPUOperand : public MCParsedAsmOperand {
 
   template <ImmTy Ty> bool isImmTy() const { return isImmTy(Ty); }
 
-  bool isImmLiteral() const { return isImmTy(ImmTyNone); }
+  bool isPlainImm() const { return isImmTy(ImmTyPlain); }
 
-  bool isImmModifier() const {
-    return isImm() && Imm.Type != ImmTyNone;
-  }
+  bool isImmModifier() const { return isImm() && !isPlainImm(); }
 
   bool isOModSI() const { return isImmTy(ImmTyOModSI); }
   bool isDim() const { return isImmTy(ImmTyDim); }
@@ -1048,12 +1046,12 @@ class AMDGPUOperand : public MCParsedAsmOperand {
   }
 
   Modifiers getModifiers() const {
-    assert(isRegKind() || isImmTy(ImmTyNone));
+    assert(isRegKind() || isPlainImm());
     return isRegKind() ? Reg.Mods : Imm.Mods;
   }
 
   void setModifiers(Modifiers Mods) {
-    assert(isRegKind() || isImmTy(ImmTyNone));
+    assert(isRegKind() || isPlainImm());
     if (isRegKind())
       Reg.Mods = Mods;
     else
@@ -1127,7 +1125,7 @@ class AMDGPUOperand : public MCParsedAsmOperand {
   static void printImmTy(raw_ostream& OS, ImmTy Type) {
     // clang-format off
     switch (Type) {
-    case ImmTyNone: OS << "None"; break;
+    case ImmTyPlain: OS << "Plain"; break;
     case ImmTyGDS: OS << "GDS"; break;
     case ImmTyLDS: OS << "LDS"; break;
     case ImmTyOffen: OS << "Offen"; break;
@@ -1211,7 +1209,7 @@ class AMDGPUOperand : public MCParsedAsmOperand {
       break;
     case Immediate:
       OS << '<' << getImm();
-      if (getImmTy() != ImmTyNone) {
+      if (!isPlainImm()) {
         OS << " type: "; printImmTy(OS, getImmTy());
       }
       OS << " mods: " << Imm.Mods << '>';
@@ -1229,7 +1227,7 @@ class AMDGPUOperand : public MCParsedAsmOperand {
 
   static AMDGPUOperand::Ptr CreateImm(const AMDGPUAsmParser *AsmParser,
                                       int64_t Val, SMLoc Loc,
-                                      ImmTy Type = ImmTyNone,
+                                      ImmTy Type = ImmTyPlain,
                                       bool IsFPImm = false) {
     auto Op = std::make_unique<AMDGPUOperand>(Immediate, AsmParser);
     Op->Imm.Val = Val;
@@ -1691,17 +1689,17 @@ class AMDGPUAsmParser : public MCTargetAsmParser {
 
   ParseStatus
   parseIntWithPrefix(const char *Prefix, OperandVector &Operands,
-                     AMDGPUOperand::ImmTy ImmTy = AMDGPUOperand::ImmTyNone,
+                     AMDGPUOperand::ImmTy ImmTy = AMDGPUOperand::ImmTyPlain,
                      std::function<bool(int64_t &)> ConvertResult = nullptr);
 
   ParseStatus parseOperandArrayWithPrefix(
       const char *Prefix, OperandVector &Operands,
-      AMDGPUOperand::ImmTy ImmTy = AMDGPUOperand::ImmTyNone,
+      AMDGPUOperand::ImmTy ImmTy = AMDGPUOperand::ImmTyPlain,
       bool (*ConvertResult)(int64_t &) = nullptr);
 
   ParseStatus
   parseNamedBit(StringRef Name, OperandVector &Operands,
-                AMDGPUOperand::ImmTy ImmTy = AMDGPUOperand::ImmTyNone);
+                AMDGPUOperand::ImmTy ImmTy = AMDGPUOperand::ImmTyPlain);
   unsigned getCPolKind(StringRef Id, StringRef Mnemo, bool &Disabling) const;
   ParseStatus parseCPol(OperandVector &Operands);
   ParseStatus parseScope(OperandVector &Operands, int64_t &Scope);
@@ -2125,7 +2123,7 @@ bool AMDGPUOperand::isInlinableImm(MVT type) const {
     return true;
   }
 
-  if (!isImmTy(ImmTyNone)) {
+  if (!isPlainImm()) {
     // Only plain immediates are inlinable (e.g. "clamp" attribute is not)
     return false;
   }
@@ -2201,7 +2199,7 @@ bool AMDGPUOperand::isInlinableImm(MVT type) const {
 
 bool AMDGPUOperand::isLiteralImm(MVT type) const {
   // Check that this immediate can be added as literal
-  if (!isImmTy(ImmTyNone)) {
+  if (!isPlainImm()) {
     return false;
   }
 
@@ -2312,7 +2310,7 @@ bool AMDGPUOperand::isBoolReg() const {
 
 uint64_t AMDGPUOperand::applyInputFPModifiers(uint64_t Val, unsigned Size) const
 {
-  assert(isImmTy(ImmTyNone) && Imm.Mods.hasFPModifiers());
+  assert(isPlainImm() && Imm.Mods.hasFPModifiers());
   assert(Size == 2 || Size == 4 || Size == 8);
 
   const uint64_t FpSignMask = (1ULL << (Size * 8 - 1));
@@ -2336,10 +2334,10 @@ void AMDGPUOperand::addImmOperands(MCInst &Inst, unsigned N, bool ApplyModifiers
   if (AMDGPU::isSISrcOperand(AsmParser->getMII()->get(Inst.getOpcode()),
                              Inst.getNumOperands())) {
     addLiteralImmOperand(Inst, Imm.Val,
-                         ApplyModifiers &
-                         isImmTy(ImmTyNone) && Imm.Mods.hasFPModifiers());
+                         ApplyModifiers & isPlainImm() &&
+                             Imm.Mods.hasFPModifiers());
   } else {
-    assert(!isImmTy(ImmTyNone) || !hasModifiers());
+    assert(!isPlainImm() || !hasModifiers());
     Inst.addOperand(MCOperand::createImm(Imm.Val));
     setImmKindNone();
   }
@@ -3345,8 +3343,8 @@ ParseStatus AMDGPUAsmParser::parseImm(OperandVector &Operands,
       RealVal.changeSign();
 
     Operands.push_back(
-      AMDGPUOperand::CreateImm(this, RealVal.bitcastToAPInt().getZExtValue(), S,
-                               AMDGPUOperand::ImmTyNone, true));
+        AMDGPUOperand::CreateImm(this, RealVal.bitcastToAPInt().getZExtValue(),
+                                 S, AMDGPUOperand::ImmTyPlain, true));
     AMDGPUOperand &Op = static_cast<AMDGPUOperand &>(*Operands.back());
     Op.setModifiers(Mods);
 
@@ -4046,7 +4044,7 @@ bool AMDGPUAsmParser::validateVOPD(const MCInst &Inst,
   if (AsVOPD3) {
     for (const std::unique_ptr<MCParsedAsmOperand> &Operand : Operands) {
       AMDGPUOperand &Op = (AMDGPUOperand &)*Operand;
-      if ((Op.isRegKind() || Op.isImmTy(AMDGPUOperand::ImmTyNone)) &&
+      if ((Op.isRegKind() || Op.isPlainImm()) &&
           (Op.getModifiers().getFPModifiersOperand() & SISrcMods::ABS))
         Error(Op.getStartLoc(), "ABS not allowed in VOPD3 instructions");
     }
@@ -9053,7 +9051,7 @@ void AMDGPUAsmParser::cvtMubufImpl(MCInst &Inst,
     }
 
     // Handle the case where soffset is an immediate
-    if (Op.isImm() && Op.getImmTy() == AMDGPUOperand::ImmTyNone) {
+    if (Op.isPlainImm()) {
       Op.addImmOperands(Inst, 1);
       continue;
     }
@@ -9078,18 +9076,18 @@ void AMDGPUAsmParser::cvtMubufImpl(MCInst &Inst,
 //===----------------------------------------------------------------------===//
 
 bool AMDGPUOperand::isSMRDOffset8() const {
-  return isImmLiteral() && isUInt<8>(getImm());
+  return isPlainImm() && isUInt<8>(getImm());
 }
 
 bool AMDGPUOperand::isSMEMOffset() const {
   // Offset range is checked later by validator.
-  return isImmLiteral();
+  return isPlainImm();
 }
 
 bool AMDGPUOperand::isSMRDLiteralOffset() const {
   // 32-bit literals are only supported on CI and we only want to use them
   // when the offset is > 8-bits.
-  return isImmLiteral() && !isUInt<8>(getImm()) && isUInt<32>(getImm());
+  return isPlainImm() && !isUInt<8>(getImm()) && isUInt<32>(getImm());
 }
 
 //===----------------------------------------------------------------------===//
@@ -9861,11 +9859,11 @@ bool AMDGPUOperand::isBLGP() const {
 }
 
 bool AMDGPUOperand::isS16Imm() const {
-  return isImmLiteral() && (isInt<16>(getImm()) || isUInt<16>(getImm()));
+  return isPlainImm() && (isInt<16>(getImm()) || isUInt<16>(getImm()));
 }
 
 bool AMDGPUOperand::isU16Imm() const {
-  return isImmLiteral() && isUInt<16>(getImm());
+  return isPlainImm() && isUInt<16>(getImm());
 }
 
 //===----------------------------------------------------------------------===//

arsenm

We seem already call them plain immediates in the code, so maybe use that.

This seems like not-ideal decay and not intentional? I think "PlainImm" is more confusing terminology. Plain compared to what?

kosarev · 2025-08-13T13:20:57Z

Plain compared to what?

To the other kinds of operands as indicated by ImmTy, which we also call immediates.

How about isNumber() then. or maybe you can suggest something? Surely keeping them as 'literals' is even less ideal?

kosarev · 2025-08-19T11:59:59Z

Ping.

jayfoad · 2025-08-19T14:00:18Z

Is ImmTyNone used both for values that can be encoded as inlines (like 3) and those that need to be literals (like 999)?

So you want a name that means something like "an immediate that is rendered as a plain old number in assembler source"?

kosarev · 2025-08-20T10:14:04Z

Is ImmTyNone used both for values that can be encoded as inlines (like 3) and those that need to be literals (like 999)?

So you want a name that means something like "an immediate that is rendered as a plain old number in assembler source"?

Yes and yes. As we create asm operands, we don't know it yet if numeric ones are going to be inline constants or what we call literals, so isImmLiteral() is misleading. Calling them none-typed immediates doesn't help reading the code either, I feel.

kosarev · 2025-08-22T09:02:59Z

Ping.

mbrkusanin · 2025-08-22T14:31:48Z

I agree that old name isImmLiteral could be misleading.

ImmTyPlain is also strange to me. I find old ImmTyNone less confusing or maybe ImmTyRegular.

kosarev requested review from jayfoad, arsenm and rampitec August 13, 2025 11:31

llvmbot added the backend:AMDGPU label Aug 13, 2025

arsenm reviewed Aug 13, 2025

View reviewed changes

kosarev requested a review from arsenm August 14, 2025 11:53

kosarev requested a review from mbrkusanin August 22, 2025 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AMDGPU][AsmParser][NFC] Give isImmLiteral() a better name. #153395

[AMDGPU][AsmParser][NFC] Give isImmLiteral() a better name. #153395

kosarev commented Aug 13, 2025

Uh oh!

llvmbot commented Aug 13, 2025

Uh oh!

arsenm left a comment

Uh oh!

kosarev commented Aug 13, 2025

Uh oh!

kosarev commented Aug 19, 2025

Uh oh!

jayfoad commented Aug 19, 2025

Uh oh!

kosarev commented Aug 20, 2025

Uh oh!

kosarev commented Aug 22, 2025

Uh oh!

mbrkusanin commented Aug 22, 2025

Uh oh!

Uh oh!

[AMDGPU][AsmParser][NFC] Give isImmLiteral() a better name. #153395

Are you sure you want to change the base?

[AMDGPU][AsmParser][NFC] Give isImmLiteral() a better name. #153395

Conversation

kosarev commented Aug 13, 2025

Uh oh!

llvmbot commented Aug 13, 2025

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

kosarev commented Aug 13, 2025

Uh oh!

kosarev commented Aug 19, 2025

Uh oh!

jayfoad commented Aug 19, 2025

Uh oh!

kosarev commented Aug 20, 2025

Uh oh!

kosarev commented Aug 22, 2025

Uh oh!

mbrkusanin commented Aug 22, 2025

Uh oh!

Uh oh!