[AMDGPU][MC] Corrected handling of "-" before expressions

[dp]

Summary

See bug 41156: https://bugs.llvm.org/show_bug.cgi?id=41156

This is the first part of a fix. This change corrects the following bug: when a constant expression starts with a "-" followed by a number, the negation operation is applied to the whole expression rather than to the first number.

For example, the sequence

s_sub_u32 s0, s0, -2+1

results in the same code as the following:

s_sub_u32 s0, s0, -3

This change also disables expressions which start with floating-point literals (with an optional sign).
Examples of enabled operands:

1.0
-1.0

Examples of disabled operands:

-1.0 + x

Without this change we cannot correctly handle negative floating-point literals like -1.0 because
llvm does not support fp operations: operands of expressions are handled as integers.
Note that:

• we cannot use llvm expressions parser for handling fp literals because unary "-" would be handled as an integer negation.
• we cannot handle unary "-" ourselves as it may start an expression (e.g. "-1.0 + x").

[artem.tamazov]

LGTM

llvm/trunk/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

OperandMatchResultTy parseOperandArrayWithPrefix(
     bool (*ConvertResult)(int64_t&) = nullptr);
 
   OperandMatchResultTy
   parseNamedBit(const char *Name, OperandVector &Operands,
                 AMDGPUOperand::ImmTy ImmTy = AMDGPUOperand::ImmTyNone);
   OperandMatchResultTy parseStringWithPrefix(StringRef Prefix,
                                              StringRef &Value);
 
-  bool parseAbsoluteExpr(int64_t &Val, bool AbsMod = false);
-  OperandMatchResultTy parseImm(OperandVector &Operands, bool AbsMod = false);
+  bool parseAbsoluteExpr(int64_t &Val, bool HasSP3AbsModifier = false);
+  OperandMatchResultTy parseImm(OperandVector &Operands, bool HasSP3AbsModifier = false);
   OperandMatchResultTy parseReg(OperandVector &Operands);
   OperandMatchResultTy parseRegOrImm(OperandVector &Operands, bool AbsMod = false);
   OperandMatchResultTy parseRegOrImmWithFPInputMods(OperandVector &Operands, bool AllowImm = true);
   OperandMatchResultTy parseRegOrImmWithIntInputMods(OperandVector &Operands, bool AllowImm = true);
   OperandMatchResultTy parseRegWithFPInputMods(OperandVector &Operands);
   OperandMatchResultTy parseRegWithIntInputMods(OperandVector &Operands);
   OperandMatchResultTy parseVReg32OrOff(OperandVector &Operands);
   OperandMatchResultTy parseDfmtNfmt(OperandVector &Operands);
private:
   bool isId(const AsmToken &Token, const StringRef Id) const;
   bool isToken(const AsmToken::TokenKind Kind) const;
   bool trySkipId(const StringRef Id);
   bool trySkipToken(const AsmToken::TokenKind Kind);
   bool skipToken(const AsmToken::TokenKind Kind, const StringRef ErrMsg);
   bool parseString(StringRef &Val, const StringRef ErrMsg = "expected a string");
   AsmToken::TokenKind getTokenKind() const;
   bool parseExpr(int64_t &Imm);
+  StringRef getTokenStr() const;
   AsmToken peekToken();
   AsmToken getToken() const;
   SMLoc getLoc() const;
   void lex();
 
 public:
   OperandMatchResultTy parseOptionalOperand(OperandVector &Operands);
   OperandMatchResultTy parseOptionalOpr(OperandVector &Operands);
if (AMDGPU::IsaInfo::hasCodeObjectV3(&getSTI())) {
     if (!updateGprCountSymbols(RegKind, DwordRegIndex, RegWidth))
       return nullptr;
   } else
     KernelScope.usesRegister(RegKind, DwordRegIndex, RegWidth);
   return AMDGPUOperand::CreateReg(this, Reg, StartLoc, EndLoc, false);
 }
 
 bool
-AMDGPUAsmParser::parseAbsoluteExpr(int64_t &Val, bool AbsMod) {
-  if (AbsMod && getLexer().peekTok().is(AsmToken::Pipe) &&
-      (getLexer().getKind() == AsmToken::Integer ||
-       getLexer().getKind() == AsmToken::Real)) {
-    // This is a workaround for handling operands like these:
+AMDGPUAsmParser::parseAbsoluteExpr(int64_t &Val, bool HasSP3AbsModifier) {
+  if (HasSP3AbsModifier) {
+    // This is a workaround for handling expressions
+    // as arguments of SP3 'abs' modifier, for example:
     //     |1.0|
     //     |-1|
+    //     |1+x|
     // This syntax is not compatible with syntax of standard
     // MC expressions (due to the trailing '|').
 
     SMLoc EndLoc;
     const MCExpr *Expr;
+    SMLoc StartLoc = getLoc();
 
     if (getParser().parsePrimaryExpr(Expr, EndLoc)) {
       return true;
     }
 
-    return !Expr->evaluateAsAbsolute(Val);
+    if (!Expr->evaluateAsAbsolute(Val))
+      return Error(StartLoc, "expected absolute expression");
+
+    return false;
   }
 
   return getParser().parseAbsoluteExpression(Val);
 }
 
 OperandMatchResultTy
-AMDGPUAsmParser::parseImm(OperandVector &Operands, bool AbsMod) {
+AMDGPUAsmParser::parseImm(OperandVector &Operands, bool HasSP3AbsModifier) {
   // TODO: add syntactic sugar for 1/(2*PI)
-  bool Minus = false;
-  if (getLexer().getKind() == AsmToken::Minus) {
-    const AsmToken NextToken = getLexer().peekTok();
-    if (!NextToken.is(AsmToken::Integer) &&
-        !NextToken.is(AsmToken::Real)) {
-        return MatchOperand_NoMatch;
-    }
-    Minus = true;
-    Parser.Lex();
+
+  const auto& Tok = getToken();
+  const auto& NextTok = peekToken();
+  bool IsReal = Tok.is(AsmToken::Real);
+  SMLoc S = Tok.getLoc();
+  bool Negate = false;
+
+  if (!IsReal && Tok.is(AsmToken::Minus) && NextTok.is(AsmToken::Real)) {
+    lex();
+    IsReal = true;
+    Negate = true;
   }
 
-  SMLoc S = Parser.getTok().getLoc();
-  switch(getLexer().getKind()) {
-  case AsmToken::Integer: {
-    int64_t IntVal;
-    if (parseAbsoluteExpr(IntVal, AbsMod))
+  if (IsReal) {
+    // Floating-point expressions are not supported.
+    // Can only allow floating-point literals with an
+    // optional sign.
+
+    StringRef Num = getTokenStr();
+    lex();
+
+    APFloat RealVal(APFloat::IEEEdouble());
+    auto roundMode = APFloat::rmNearestTiesToEven;
+    if (RealVal.convertFromString(Num, roundMode) == APFloat::opInvalidOp) {
       return MatchOperand_ParseFail;
-    if (Minus)
-      IntVal *= -1;
-    Operands.push_back(AMDGPUOperand::CreateImm(this, IntVal, S));
-    return MatchOperand_Success;
     }
-  case AsmToken::Real: {
-    int64_t IntVal;
-    if (parseAbsoluteExpr(IntVal, AbsMod))
-      return MatchOperand_ParseFail;
+    if (Negate)
+      RealVal.changeSign();
 
-    APFloat F(BitsToDouble(IntVal));
-    if (Minus)
-      F.changeSign();
     Operands.push_back(
-        AMDGPUOperand::CreateImm(this, F.bitcastToAPInt().getZExtValue(), S,
-                                 AMDGPUOperand::ImmTyNone, true));
+      AMDGPUOperand::CreateImm(this, RealVal.bitcastToAPInt().getZExtValue(), S,
+
+    return MatchOperand_Success;
+
+    // FIXME: Should enable arbitrary expressions here
+  } else if (Tok.is(AsmToken::Integer) ||
+             (Tok.is(AsmToken::Minus) && NextTok.is(AsmToken::Integer))){
+
+    int64_t IntVal;
+    if (parseAbsoluteExpr(IntVal, HasSP3AbsModifier))
+      return MatchOperand_ParseFail;
+
+    Operands.push_back(AMDGPUOperand::CreateImm(this, IntVal, S));
     return MatchOperand_Success;
   }
-  default:
-    return MatchOperand_NoMatch;
-  }
-}
+
 
 OperandMatchResultTy
 AMDGPUAsmParser::parseReg(OperandVector &Operands) {
   if (!isRegister())
     return MatchOperand_NoMatch;
 
   if (auto R = parseRegister()) {
     assert(R->isReg());
AMDGPUAsmParser::getTokenKind() const {
   return getLexer().getKind();
 }
 
 SMLoc
 AMDGPUAsmParser::getLoc() const {
   return getToken().getLoc();
 }
 
+StringRef
+AMDGPUAsmParser::getTokenStr() const {
+  return getToken().getString();
+}
+
 void
 AMDGPUAsmParser::lex() {
   Parser.Lex();
 }
 
 //===----------------------------------------------------------------------===//
 // swizzle
 //===----------------------------------------------------------------------===//

llvm/trunk/test/MC/AMDGPU/expressions.s

-// RUN: llvm-mc -arch=amdgcn -mcpu=fiji -show-encoding %s | FileCheck %s --check-prefix=VI
+// RUN: not llvm-mc -arch=amdgcn -mcpu=fiji -show-encoding %s | FileCheck %s --check-prefix=VI
+// RUN: not llvm-mc -arch=amdgcn -mcpu=fiji -show-encoding %s 2>&1 | FileCheck %s --check-prefix=NOVI
 
 
 .globl global
 .globl gds
 
 // Parse a global expression
 s_mov_b32 s0, global
 // VI: s_mov_b32 s0, global ; encoding: [0xff,0x00,0x80,0xbe,A,A,A,A]

 
 t=1
 s_sub_u32 s0, s0, -t
 // VI: s_sub_u32 s0, s0, -1            ; encoding: [0x00,0xc1,0x80,0x80]
 
 t=-1
 s_sub_u32 s0, s0, -t
 // VI: s_sub_u32 s0, s0, 1             ; encoding: [0x00,0x81,0x80,0x80]
+
+s_sub_u32 s0, s0, -2+1
+// VI: s_sub_u32 s0, s0, -1            ; encoding: [0x00,0xc1,0x80,0x80]
+
+t=1
+s_sub_u32 s0, s0, -2+t
+// VI: s_sub_u32 s0, s0, -1            ; encoding: [0x00,0xc1,0x80,0x80]
+
+s_sub_u32 s0, s0, -1.0 + 10000000000
+// NOVI: error: invalid operand for instruction
+
+t=10000000000
+s_sub_u32 s0, s0, 1.0 + t
+// NOVI: error: invalid operand for instruction