[AMDGPU] Add lit() asm operand modifier for SP3 compatibility.

[rampitec]

A literal can be optionally wrapped in a lit() expression. This does not force literal encoding for an inline immediate, but neither does SP3. The syntax is dummy for compatibility only.

[llvmbot]

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-amdgpu

Author: Stanislav Mekhanoshin (rampitec)

Changes

A literal can be optionally wrapped in a lit() expression. This does not force literal encoding for an inline immediate, but neither does SP3. The syntax is dummy for compatibility only.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp (+15-3)
• (modified) llvm/test/MC/AMDGPU/literals.s (+36)

diff --git a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
index 35656bcaea1af7f..1e07e8deb560fcb 100644
--- a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
+++ b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
@@ -75,6 +75,7 @@ class AMDGPUOperand : public MCParsedAsmOperand {
     bool Abs = false;
     bool Neg = false;
     bool Sext = false;
+    bool Lit = false;
 
     bool hasFPModifiers() const { return Abs || Neg; }
     bool hasIntModifiers() const { return Sext; }
@@ -3005,7 +3006,7 @@ bool
 AMDGPUAsmParser::isNamedOperandModifier(const AsmToken &Token, const AsmToken &NextToken) const {
   if (Token.is(AsmToken::Identifier) && NextToken.is(AsmToken::LParen)) {
     const auto &str = Token.getString();
-    return str == "abs" || str == "neg" || str == "sext";
+    return str == "abs" || str == "neg" || str == "sext" || str == "lit";
   }
   return false;
 }
@@ -3094,6 +3095,7 @@ AMDGPUAsmParser::parseRegOrImmWithFPInputMods(OperandVector &Operands,
                                               bool AllowImm) {
   bool Neg, SP3Neg;
   bool Abs, SP3Abs;
+  bool Lit;
   SMLoc Loc;
 
   // Disable ambiguous constructs like '--1' etc. Should use neg(-1) instead.
@@ -3113,6 +3115,10 @@ AMDGPUAsmParser::parseRegOrImmWithFPInputMods(OperandVector &Operands,
   if (Abs && !skipToken(AsmToken::LParen, "expected left paren after abs"))
     return ParseStatus::Failure;
 
+  Lit = trySkipId("lit");
+  if (Lit && !skipToken(AsmToken::LParen, "expected left paren after lit"))
+    return ParseStatus::Failure;
+
   Loc = getLoc();
   SP3Abs = trySkipToken(AsmToken::Pipe);
   if (Abs && SP3Abs)
@@ -3125,7 +3131,10 @@ AMDGPUAsmParser::parseRegOrImmWithFPInputMods(OperandVector &Operands,
     Res = parseReg(Operands);
   }
   if (!Res.isSuccess())
-    return (SP3Neg || Neg || SP3Abs || Abs) ? ParseStatus::Failure : Res;
+    return (SP3Neg || Neg || SP3Abs || Abs || Lit) ? ParseStatus::Failure : Res;
+
+  if (Lit && !Operands.back()->isImm())
+    Error(Loc, "expected immediate with lit modifier");
 
   if (SP3Abs && !skipToken(AsmToken::Pipe, "expected vertical bar"))
     return ParseStatus::Failure;
@@ -3133,12 +3142,15 @@ AMDGPUAsmParser::parseRegOrImmWithFPInputMods(OperandVector &Operands,
     return ParseStatus::Failure;
   if (Neg && !skipToken(AsmToken::RParen, "expected closing parentheses"))
     return ParseStatus::Failure;
+  if (Lit && !skipToken(AsmToken::RParen, "expected closing parentheses"))
+    return ParseStatus::Failure;
 
   AMDGPUOperand::Modifiers Mods;
   Mods.Abs = Abs || SP3Abs;
   Mods.Neg = Neg || SP3Neg;
+  Mods.Lit = Lit;
 
-  if (Mods.hasFPModifiers()) {
+  if (Mods.hasFPModifiers() || Lit) {
     AMDGPUOperand &Op = static_cast<AMDGPUOperand &>(*Operands.back());
     if (Op.isExpr())
       return Error(Op.getStartLoc(), "expected an absolute expression");
diff --git a/llvm/test/MC/AMDGPU/literals.s b/llvm/test/MC/AMDGPU/literals.s
index 8e5e8fd1886f416..910e3d82b2fc849 100644
--- a/llvm/test/MC/AMDGPU/literals.s
+++ b/llvm/test/MC/AMDGPU/literals.s
@@ -889,3 +889,39 @@ v_pk_add_f16 v255, private_base, private_limit
 // NOSICIVI: :[[@LINE+2]]:{{[0-9]+}}: error: instruction not supported on this GPU
 // NOGFX9: :[[@LINE+1]]:{{[0-9]+}}: error: invalid operand (violates constant bus restrictions)
 v_pk_add_f16 v255, vccz, execz
+
+//---------------------------------------------------------------------------//
+// check dummy lit() syntax for sp3 compatibility.
+//---------------------------------------------------------------------------//
+
+// SICI: v_sqrt_f32_e32 v2, 0x7b                 ; encoding: [0xff,0x66,0x04,0x7e,0x7b,0x00,0x00,0x00]
+// GFX89: v_sqrt_f32_e32 v2, 0x7b                 ; encoding: [0xff,0x4e,0x04,0x7e,0x7b,0x00,0x00,0x00]
+v_sqrt_f32 v2, lit(123)
+
+// SICI: v_sqrt_f32_e32 v2, 0x7b                 ; encoding: [0xff,0x66,0x04,0x7e,0x7b,0x00,0x00,0x00]
+// GFX89: v_sqrt_f32_e32 v2, 0x7b                 ; encoding: [0xff,0x4e,0x04,0x7e,0x7b,0x00,0x00,0x00]
+v_sqrt_f32 v2, abs(lit(123))
+
+// SICI: v_sqrt_f32_e32 v2, 0x42f60000           ; encoding: [0xff,0x66,0x04,0x7e,0x00,0x00,0xf6,0x42
+// GFX89: v_sqrt_f32_e32 v2, 0x42f60000           ; encoding: [0xff,0x4e,0x04,0x7e,0x00,0x00,0xf6,0x42]
+v_sqrt_f32 v2, lit(123.0)
+
+// SICI: v_sqrt_f64_e32 v[2:3], 0x405ec000       ; encoding: [0xff,0x68,0x04,0x7e,0x00,0xc0,0x5e,0x40]
+// GFX89: v_sqrt_f64_e32 v[2:3], 0x405ec000       ; encoding: [0xff,0x50,0x04,0x7e,0x00,0xc0,0x5e,0x40]
+v_sqrt_f64 v[2:3], lit(123.0)
+
+// SICI: v_sqrt_f64_e32 v[2:3], 0x7b             ; encoding: [0xff,0x68,0x04,0x7e,0x7b,0x00,0x00,0x00]
+// GFX89: v_sqrt_f64_e32 v[2:3], 0x7b             ; encoding: [0xff,0x50,0x04,0x7e,0x7b,0x00,0x00,0x00]
+v_sqrt_f64 v[2:3], lit(123)
+
+// NOSICI: :[[@LINE+2]]:{{[0-9]+}}: error: expected left paren after lit
+// NOGFX89: :[[@LINE+1]]:{{[0-9]+}}: error: expected left paren after lit
+v_sqrt_f32 v2, lit 123.0
+
+// NOSICI: :[[@LINE+2]]:{{[0-9]+}}: error: expected closing parentheses
+// NOGFX89: :[[@LINE+1]]:{{[0-9]+}}: error: expected closing parentheses
+v_sqrt_f32 v2, lit(123.0
+
+// NOSICI: :[[@LINE+2]]:{{[0-9]+}}: error: expected immediate with lit modifier
+// NOGFX89: :[[@LINE+1]]:{{[0-9]+}}: error: expected immediate with lit modifier
+v_sqrt_f32 v2, lit(v1)

[jayfoad]

This does not force literal encoding for an inline immediate, but neither does SP3. The syntax is dummy for compatibility only.

Thanks. This sounds OK to me. Really we should go one better than SP3 and force literal encoding, but I guess that is hard to implement.

Do you think the disassembler should emit lit(), either for all literal operands or at least for ones that could have been encoded as inlines?

Does your implementation accept lit(abs(123.0))? Does SP3 accept that?

[jayfoad]

Today I learned that we already accept abs(...) and neg(...). I had no idea!

[rampitec]

Yeah, we do. Yet another compatibility thing. There is a special handling of the neg(abs()), the only useful combination. If we ever want to handle lit(abs()) it would need a similar special case handling.

[rampitec]

This does not force literal encoding for an inline immediate, but neither does SP3. The syntax is dummy for compatibility only.

Thanks. This sounds OK to me. Really we should go one better than SP3 and force literal encoding, but I guess that is hard to implement.

Yes, it needs:

1. Extra bit in the MCOperand. This should be a per-operand state.
2. More logic in the constant bus checks.

This could be theoretically added later, but not required for compatibility.

Do you think the disassembler should emit lit(), either for all literal operands or at least for ones that could have been encoded as inlines?

Unless we enforce literal encoding I do not see a reason to clutter the output.

Does your implementation accept lit(abs(123.0))? Does SP3 accept that?

This does not support it:

llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding  <<< 'v_sqrt_f32 v2, lit(abs(123.0))'
	.text
<stdin>:1:20: error: failed parsing operand.
v_sqrt_f32 v2, lit(abs(123.0))
                   ^

SP3 does, although there is no abs modifier used anyway. It might be possible to support, but I see no usecases. On practice people just use lit(). Actually since SP3 does not do it I see no way to check if HW even supports modifier bits with literals. So far lit() is just a sort of syntax glue.

[rampitec]

Does your implementation accept lit(abs(123.0))? Does SP3 accept that?

This does not support it:

llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding  <<< 'v_sqrt_f32 v2, lit(abs(123.0))'
	.text
<stdin>:1:20: error: failed parsing operand.
v_sqrt_f32 v2, lit(abs(123.0))
                   ^

SP3 does, although there is no abs modifier used anyway. It might be possible to support, but I see no usecases. On practice people just use lit(). Actually since SP3 does not do it I see no way to check if HW even supports modifier bits with literals. So far lit() is just a sort of syntax glue.

BTW, it does support the opposite:

llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding  <<< 'v_sqrt_f32 v2, abs(lit(-123.0))'
	.text
	v_sqrt_f32_e32 v2, 0x42f60000           ; encoding: [0xff,0x4e,0x04,0x7e,0x00,0x00,0xf6,0x42]

It makes more sense to me: use literal and abs bit on that literal. The abs(<num>) thing is constant evaluated. Just in case if we support actual literal encoding one day. I do not really understand a meaning of lit(abs()) other than the ask to constant evaluate a number and then use as a literal.

[jayfoad]

Does your implementation accept lit(abs(123.0))? Does SP3 accept that?

This does not support it:

llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding  <<< 'v_sqrt_f32 v2, lit(abs(123.0))'
	.text
<stdin>:1:20: error: failed parsing operand.
v_sqrt_f32 v2, lit(abs(123.0))
                   ^

Good. I think the operand of lit() should just be a constant. It should not be allowed to include any other modifiers.

[rampitec]

It makes more sense to me: use literal and abs bit on that literal. The abs(<num>) thing is constant evaluated. Just in case if we support actual literal encoding one day. I do not really understand a meaning of lit(abs()) other than the ask to constant evaluate a number and then use as a literal.

Moreover, neg(abs()) is supported. abs(neg()) is not. Which also makes sense, as you cannot really do abs(neg()) in a non-constant case:

lvm-mc -arch=amdgcn -mcpu=tonga -show-encoding  <<< 'v_sqrt_f32 v2, abs(neg(-123.0))'
	.text
<stdin>:1:20: error: failed parsing operand.
v_sqrt_f32 v2, abs(neg(-123.0))
                   ^
llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding  <<< 'v_sqrt_f32 v2, neg(abs(-123.0))'
	.text
	v_sqrt_f32_e32 v2, 0xc2f60000           ; encoding: [0xff,0x4e,0x04,0x7e,0x00,0x00,0xf6,0xc2]

And then:

llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding  <<< 'v_sqrt_f32 v2, neg(abs(lit(-123.0)))'
	.text
	v_sqrt_f32_e32 v2, 0xc2f60000           ; encoding: [0xff,0x4e,0x04,0x7e,0x00,0x00,0xf6,0xc2]

I.e. useful and meaningful combinations are supported.

[jayfoad]

abs(neg()) is not. Which also makes sense, as you cannot really do abs(neg()) in a non-constant case

abs(neg(x)) could be treated as abs(x).

[rampitec]

abs(neg()) is not. Which also makes sense, as you cannot really do abs(neg()) in a non-constant case

abs(neg(x)) could be treated as abs(x).

It probably could, but it sounds like an user error, and then asm silently accepting this error. After all asm is not a high level language where it could be justified. I can imagine this sort of constructs created by a pre-processor, but I do not really think an asm writer expects this trap. Asm is not a tool to prevent an user to shot himself in a foot, but creating such traps is not its goal either.

[rampitec]

On a practical note I can see 2 use cases where it may be useful:

1. Force lit encoding to be patched as a relocation. We have a sort of w/a for this, but not at asm level. Again, this boils down to the available representation bits. MI Operand has target bits, but MCOperand does not. So far I do not see a strong evidence we need to make MCOperand heavier than it is now.
2. HW testing of the modifiers applied to literals. Although this does not sound very useful. At the end of the day HW must be tested with SP3 and SP3 must lead here.

[kosarev]

Good. I think the operand of lit() should just be a constant. It should not be allowed to include any other modifiers.

Still might be useful to allow (at some point) expressions within?

Force lit encoding to be patched as a relocation. We have a sort of w/a for this, but not at asm level. Again, this boils down to the available representation bits. MI Operand has target bits, but MCOperand does not.

Well, we can have our own AMDGPUMCExprs.

abs(neg(x)) could be treated as abs(x).

So far I was under impression that these modifiers are supposed to designate corresponding encoding bits, just as our id[:value] instruction modifiers do, in which case the expectation would probably be to not allow combinations that are not supported by the target hardware.

Otherwise, if we see these more like generic expressions, then maybe using the good old minus sign would make more sense than neg()?

[jayfoad]

abs(neg(x)) could be treated as abs(x).

So far I was under impression that these modifiers are supposed to designate corresponding encoding bits, just as our id[:value] instruction modifiers do, in which case the expectation would probably be to not allow combinations that are not supported by the target hardware.

Otherwise, if we see these more like generic expressions, then maybe using the good old minus sign would make more sense than neg()?

Fair point. I am happy for abs(neg(x)) to be an assembler error.

[rampitec]

abs(neg(x)) could be treated as abs(x).

So far I was under impression that these modifiers are supposed to designate corresponding encoding bits, just as our id[:value] instruction modifiers do, in which case the expectation would probably be to not allow combinations that are not supported by the target hardware.
Otherwise, if we see these more like generic expressions, then maybe using the good old minus sign would make more sense than neg()?

Fair point. I am happy for abs(neg(x)) to be an assembler error.

Technically -123.0 and neg(123.0) are different things. First is a negative constant. Second is a positive constant and a modifier. But noone does it, not even SP3.