Feedback, add check for number of commas with tests

Author: JanekvO

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

@@ -8296,6 +8296,7 @@ bool AMDGPUAsmParser::parsePrimaryExpr(const MCExpr *&Res, SMLoc &EndLoc) {
 
     if (VK != AGVK::AGVK_None && peekToken().is(AsmToken::LParen)) {
       SmallVector<const MCExpr *, 4> Exprs;
+      uint64_t CommaCount = 0;
       lex(); // Eat 'max'/'or'
       lex(); // Eat '('
       while (true) {
@@ -8305,14 +8306,22 @@ bool AMDGPUAsmParser::parsePrimaryExpr(const MCExpr *&Res, SMLoc &EndLoc) {
                   "empty " + Twine(TokenId) + " expression");
             return true;
           }
+          if (CommaCount + 1 != Exprs.size()) {
+            Error(getToken().getLoc(),
+                  "mismatch of commas in " + Twine(TokenId) + " expression");
+            return true;
+          }
           Res = AMDGPUVariadicMCExpr::create(VK, Exprs, getContext());
           return false;
         }
         const MCExpr *Expr;
         if (getParser().parseExpression(Expr, EndLoc))
           return true;
         Exprs.push_back(Expr);
-        if (!trySkipToken(AsmToken::Comma) && !isToken(AsmToken::RParen)) {
+        bool LastTokenWasComma = trySkipToken(AsmToken::Comma);
+        if (LastTokenWasComma)
+          CommaCount++;
+        if (!LastTokenWasComma && !isToken(AsmToken::RParen)) {
           Error(getToken().getLoc(),
                 "unexpected token in " + Twine(TokenId) + " expression");
           return true;

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCExpr.cpp

@@ -49,20 +49,21 @@ void AMDGPUVariadicMCExpr::printImpl(raw_ostream &OS,
   OS << ')';
 }
 
+static int64_t Op(AMDGPUVariadicMCExpr::AMDGPUVariadicKind Kind, int64_t Arg1,
+                  int64_t Arg2) {
+  switch (Kind) {
+  default:
+    llvm_unreachable("Unknown AMDGPUVariadicMCExpr kind.");
+  case AMDGPUVariadicMCExpr::AMDGPUVariadicKind::AGVK_Max:
+    return std::max(Arg1, Arg2);
+  case AMDGPUVariadicMCExpr::AMDGPUVariadicKind::AGVK_Or:
+    return Arg1 || Arg2;
+  }
+}
+
 bool AMDGPUVariadicMCExpr::evaluateAsRelocatableImpl(
     MCValue &Res, const MCAsmLayout *Layout, const MCFixup *Fixup) const {
-  std::optional<int64_t> Total = {};
-
-  auto Op = [this](int64_t Arg1, int64_t Arg2) -> int64_t {
-    switch (Kind) {
-    default:
-      llvm_unreachable("Unknown AMDGPUVariadicMCExpr kind.");
-    case AGVK_Max:
-      return std::max(Arg1, Arg2);
-    case AGVK_Or:
-      return Arg1 || Arg2;
-    }
-  };
+  std::optional<int64_t> Total;
 
   for (const MCExpr *Arg : Args) {
     MCValue ArgRes;
@@ -72,7 +73,7 @@ bool AMDGPUVariadicMCExpr::evaluateAsRelocatableImpl(
 
     if (!Total.has_value())
       Total = ArgRes.getConstant();
-    Total = Op(*Total, ArgRes.getConstant());
+    Total = Op(Kind, *Total, ArgRes.getConstant());
   }
 
   Res = MCValue::get(*Total);

llvm/test/MC/AMDGPU/mcexpr_amd_err.s

@@ -1,29 +1,44 @@
 // RUN: not llvm-mc -triple amdgcn-amd-amdhsa %s 2>&1 | FileCheck --check-prefix=ASM %s
 
-// ASM: 20:22: error: empty max expression
-// ASM: 20:22: error: missing expression
-// ASM: 21:20: error: empty or expression
-// ASM: 21:20: error: missing expression
-// ASM: 23:29: error: unknown token in expression
-// ASM: 23:29: error: missing expression
-// ASM: 24:32: error: unexpected token in max expression
-// ASM: 24:32: error: missing expression
-// ASM: 25:42: error: unknown token in expression
-// ASM: 25:42: error: missing expression
-// ASM: 26:38: error: unexpected token in or expression
-// ASM: 26:38: error: missing expression
-
 .set one, 1
 .set two, 2
 .set three, 3
 
 .set max_empty, max()
+// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: empty max expression
+// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
+
 .set or_empty, or()
+// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: empty or expression
+// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
+
 .set max_post_aux_comma, max(one,)
+// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: mismatch of commas in max expression
+// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
+
 .set max_pre_aux_comma, max(,one)
+// asm: :[[@line-1]]:{{[0-9]+}}: error: unknown token in expression
+// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
+
+.set max_double_comma, max(one,, two)
+// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: unknown token in expression
+// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
+
+.set max_no_comma, max(one two)
+// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: unexpected token in max expression
+// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
+
 .set max_missing_paren, max(two
+// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: unexpected token in max expression
+// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
+
 .set max_expression_one, max(three, four,
+// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: unknown token in expression
+// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
+
 .set or_expression_one, or(four, five
+// ASM: :[[@LINE-1]]:{{[0-9]+}}: error: unexpected token in or expression
+// ASM: :[[@LINE-2]]:{{[0-9]+}}: error: missing expression
 
 .set four, 4
 .set five, 5