[AMDGPU][MC] Corrected parsing of SP3 'neg' modifier

[dp]

Summary

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

This is the second part of a fix. This change corrects the following bug: constant expressions started with "-" followed by a symbol are evaluated incorrectly: the "-" sign is simply ignored.

For example, the sequence

.set foo, 1
v_mul_f32 v0, -foo+2, v2  // -foo+2 = 1?

results in the same code as the following:

v_mul_f32 v0, 3, v2

[artem.tamazov]

LGTM

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

public:
 
   OperandMatchResultTy
   parseNamedBit(const char *Name, OperandVector &Operands,
                 AMDGPUOperand::ImmTy ImmTy = AMDGPUOperand::ImmTyNone);
   OperandMatchResultTy parseStringWithPrefix(StringRef Prefix,
                                              StringRef &Value);
 
   bool parseAbsoluteExpr(int64_t &Val, bool HasSP3AbsModifier = false);
+  bool parseSP3NegModifier();
   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);
private:
 
   bool isId(const StringRef Id) const;
   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");
+  void peekTokens(MutableArrayRef<AsmToken> Tokens);
   AsmToken::TokenKind getTokenKind() const;
   bool parseExpr(int64_t &Imm);
   StringRef getTokenStr() const;
   AsmToken peekToken();
   AsmToken getToken() const;
   SMLoc getLoc() const;
   void lex();
 

 OperandMatchResultTy
 AMDGPUAsmParser::parseRegOrImm(OperandVector &Operands, bool AbsMod) {
   auto res = parseReg(Operands);
   return (res == MatchOperand_NoMatch)?
          parseImm(Operands, AbsMod) :
          res;
 }
 
+// Check if the current token is an SP3 'neg' modifier.
+// Currently this modifier is allowed in the following context:
+//
+// 1. Before a register, e.g. "-v0", "-v[...]" or "-[v0,v1]".
+// 2. Before an 'abs' modifier: -abs(...)
+// 3. Before an SP3 'abs' modifier: -|...|
+//
+// In all other cases "-" is handled as a part
+// of an expression that follows the sign.
+//
+// Note: When "-" is followed by an integer literal,
+// this is interpreted as integer negation rather
+// than a floating-point NEG modifier applied to N.
+// Beside being contr-intuitive, such use of floating-point
+// NEG modifier would have resulted in different meaning
+// of integer literals used with VOP1/2/C and VOP3,
+// for example:
+//    v_exp_f32_e32 v5, -1 // VOP1: src0 = 0xFFFFFFFF
+//    v_exp_f32_e64 v5, -1 // VOP3: src0 = 0x80000001
+// Negative fp literals with preceding "-" are
+// handled likewise for unifomtity
+//
+bool
+AMDGPUAsmParser::parseSP3NegModifier() {
+
+  AsmToken NextToken[2];
+  peekTokens(NextToken);
+
+  if (isToken(AsmToken::Minus) &&
+      (isRegister(NextToken[0], NextToken[1]) ||
+       NextToken[0].is(AsmToken::Pipe) ||
+       isId(NextToken[0], "abs"))) {
+    lex();
+    return true;
+  }
+
+  return false;
+}
+
 OperandMatchResultTy
 AMDGPUAsmParser::parseRegOrImmWithFPInputMods(OperandVector &Operands,
                                               bool AllowImm) {
-  bool Negate = false, Negate2 = false, Abs = false, Abs2 = false;
-
-  if (getLexer().getKind()== AsmToken::Minus) {
-    const AsmToken NextToken = getLexer().peekTok();
+  bool Negate, Negate2 = false, Abs = false, Abs2 = false;
 
-    // Disable ambiguous constructs like '--1' etc. Should use neg(-1) instead.
-    if (NextToken.is(AsmToken::Minus)) {
-      Error(Parser.getTok().getLoc(), "invalid syntax, expected 'neg' modifier");
-      return MatchOperand_ParseFail;
-    }
+  if (isToken(AsmToken::Minus) && peekToken().is(AsmToken::Minus)) {
+    Error(getLoc(), "invalid syntax, expected 'neg' modifier");
 
-    // '-' followed by an integer literal N should be interpreted as integer
-    // negation rather than a floating-point NEG modifier applied to N.
-    // Beside being contr-intuitive, such use of floating-point NEG modifier
-    // results in different meaning of integer literals used with VOP1/2/C
-    // and VOP3, for example:
-    //    v_exp_f32_e32 v5, -1 // VOP1: src0 = 0xFFFFFFFF
-    //    v_exp_f32_e64 v5, -1 // VOP3: src0 = 0x80000001
-    // Negative fp literals should be handled likewise for unifomtity
-    if (!NextToken.is(AsmToken::Integer) && !NextToken.is(AsmToken::Real)) {
-      Parser.Lex();
-      Negate = true;
-    }
-  }
+  Negate = parseSP3NegModifier();
 
   if (getLexer().getKind() == AsmToken::Identifier &&
       Parser.getTok().getString() == "neg") {
     if (Negate) {
       Error(Parser.getTok().getLoc(), "expected register or immediate");
       return MatchOperand_ParseFail;
     }
     Parser.Lex();
AMDGPUAsmParser::parseRegOrImmWithFPInputMods(OperandVector &Operands,
 
   OperandMatchResultTy Res;
   if (AllowImm) {
     Res = parseRegOrImm(Operands, Abs);
   } else {
     Res = parseReg(Operands);
   }
   if (Res != MatchOperand_Success) {
-    return Res;
+    return (Negate || Negate2 || Abs || Abs2)? MatchOperand_ParseFail : Res;
   }
 
   AMDGPUOperand::Modifiers Mods;
   if (Abs) {
     if (getLexer().getKind() != AsmToken::Pipe) {
       Error(Parser.getTok().getLoc(), "expected vertical bar");
       return MatchOperand_ParseFail;
     }
AMDGPUAsmParser::parseRegOrImmWithIntInputMods(OperandVector &Operands,
 
   OperandMatchResultTy Res;
   if (AllowImm) {
     Res = parseRegOrImm(Operands);
   } else {
     Res = parseReg(Operands);
   }
   if (Res != MatchOperand_Success) {
-    return Res;
+    return Sext? MatchOperand_ParseFail : Res;
   }
 
   AMDGPUOperand::Modifiers Mods;
   if (Sext) {
     if (getLexer().isNot(AsmToken::RParen)) {
       Error(Parser.getTok().getLoc(), "expected closing parentheses");
       return MatchOperand_ParseFail;
     }
AMDGPUAsmParser::getToken() const {
   return Parser.getTok();
 }
 
 AsmToken
 AMDGPUAsmParser::peekToken() {
   return getLexer().peekTok();
 }
 
+void
+AMDGPUAsmParser::peekTokens(MutableArrayRef<AsmToken> Tokens) {
+  auto TokCount = getLexer().peekTokens(Tokens);
+
+  for (auto Idx = TokCount; Idx < Tokens.size(); ++Idx)
+    Tokens[Idx] = AsmToken(AsmToken::Error, "");
+}
+
 AsmToken::TokenKind
 AMDGPUAsmParser::getTokenKind() const {
   return getLexer().getKind();
 }
 
 SMLoc
 AMDGPUAsmParser::getLoc() const {
   return getToken().getLoc();

test/MC/AMDGPU/expressions.s

 // 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
+
+//===----------------------------------------------------------------------===//
+// Symbols may look like registers.
+// They should be allowed in expressions if there is no ambiguity.
+//===----------------------------------------------------------------------===//
+
+v=1
+v_sin_f32 v0, -v
+// VI: v_sin_f32_e32 v0, -1            ; encoding: [0xc1,0x52,0x00,0x7e]
+
+v_sin_f32 v0, -v[0]
+// VI: v_sin_f32_e64 v0, -v0           ; encoding: [0x00,0x00,0x69,0xd1,0x00,0x01,0x00,0x20]
+
+s=1
+v_sin_f32 v0, -s
+// VI: v_sin_f32_e32 v0, -1            ; encoding: [0xc1,0x52,0x00,0x7e]
+
+s0=1
+v_sin_f32 v0, -s0
+// VI: v_sin_f32_e64 v0, -s0           ; encoding: [0x00,0x00,0x69,0xd1,0x00,0x00,0x00,0x20]
+
+ttmp=1
+v_sin_f32 v0, -ttmp
+// VI: v_sin_f32_e32 v0, -1            ; encoding: [0xc1,0x52,0x00,0x7e]
+
+ttmp0=1
+v_sin_f32 v0, -[ttmp0]
+// VI: v_sin_f32_e64 v0, -ttmp0        ; encoding: [0x00,0x00,0x69,0xd1,0x70,0x00,0x00,0x20]

test/MC/AMDGPU/vop3-modifiers-err.s

 // RUN: not llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding %s 2>&1 | FileCheck %s
 
 //---------------------------------------------------------------------------//
 // VOP3 Modifiers
 //---------------------------------------------------------------------------//
 
 // 'neg(1)' cannot be encoded as 32-bit literal while preserving e64 semantics
 v_ceil_f64_e32 v[0:1], neg(1)
 // CHECK: error: invalid operand for instruction
 
 v_ceil_f32 v0, --1
 // CHECK: error: invalid syntax, expected 'neg' modifier
 
 v_ceil_f16 v0, abs(neg(1))
-// CHECK: error: not a valid operand
+// CHECK: error: failed parsing operand