[AsmParser][AVX512]Enhance OpMask/Zero/Merge syntax check rubostness

[coby]

Summary

Adopt a more strict approach regarding what marks should/can appear after a destination register, when operating upon an AVX512 platform.

[coby]

ping

[m_zuckerman]

test/MC/X86/avx512-err.s

10

why is this error a true?
according to the ISA, the instruction can optionally have op mask.

[m_zuckerman]

Expect (see above ) from that Look good to me

[coby]

test/MC/X86/avx512-err.s

10

The error due to the misplacing of the rounding (rn-sae) mark (line 11).
Only an OpMask / Zero mark is allowed to immediately follow a destination operand.

[m_zuckerman]

test/MC/X86/avx512-err.s

8

So why this is an error? you mustn't have the {z} mask before the {rn-sae}.
Again according to the ISA section 2.5.4
https://software.intel.com/sites/default/files/managed/07/b7/319433-023.pdf
for example:
vaddps zmm7 {k6}, zmm2, zmm4, {rd-sae}

10

can you add also the intel syntax test since it is written in a different way? The location of the mask is the first and not the last.

For example:
vaddps zmm7 {k6}, zmm2, zmm4, {rd-sae}

and see that you are getting the same error when you insert
vaddps zmm7 , zmm2, zmm4, {rd-sae}

[m_zuckerman]

test/MC/X86/avx512-err.s

8

you don't must the {z} before the {rn-sae}

[coby]

test/MC/X86/avx512-err.s

8

Examine the example you took out of the Spec:
"vaddps zmm7 {k6}, zmm2, zmm4, {rd-sae}" (Intel style)
The rounding mark is stated as a distinctive operand of the instruction (note the comma).
As said above - only OpMask/Zero marks can immediately follow a destination
immediately ::= As a mark stating properties of the followed operand (or, if you want, not as a distinctive operand)

10

The code flow dictates that this added check will occur after any operand of an AVX512 instruction (more precisely - if this operand is followed by an alleged curly braces marks), so it catches both mischief conducted on Intel and GNU. In other words - it matters not whether the faulty occurs on the last/first operand

[coby]

Added intel-dialect test as well

lib/Target/X86/AsmParser/X86AsmParser.cpp

if (getLexer().is(AsmToken::LCurly)) {
         // Currently, we're just pass a '{' mark
         std::unique_ptr<X86Operand> Z;
         if (ParseZ(Z, consumedToken))
           return true;
         // Reaching here means that parsing of the allegadly '{z}' mark yielded
         // no errors.
         // Query for the need of further parsing for a {%k<NUM>} mark
         if (!Z || getLexer().is(AsmToken::LCurly)) {
-          const SMLoc StartLoc = Z ? consumeToken() : consumedToken;
+          SMLoc StartLoc = Z ? consumeToken() : consumedToken;
           // Parse an op-mask register mark ({%k<NUM>}), which is now to be
           // expected
-          if (std::unique_ptr<X86Operand> Op = ParseOperand()) {
+          unsigned RegNo;
+          if (!ParseRegister(RegNo, StartLoc, StartLoc) &&
+              X86MCRegisterClasses[X86::VK1RegClassID].contains(RegNo)) {
             if (!getLexer().is(AsmToken::RCurly))
               return Error(getLexer().getLoc(), "Expected } at this point");
             Operands.push_back(X86Operand::CreateToken("{", StartLoc));
-            Operands.push_back(std::move(Op));
+            Operands.push_back(std::move(X86Operand::CreateReg(RegNo, StartLoc,
+                                                               StartLoc)));
             Operands.push_back(X86Operand::CreateToken("}", consumeToken()));
           } else
             return Error(getLexer().getLoc(),
                           "Expected an op-mask register at this point");
           // {%k<NUM>} mark is found, inquire for {z}
           if (getLexer().is(AsmToken::LCurly) && !Z) {
             // Have we've found a parsing error, or found no (expected) {z} mark
             // - report an error
             if (ParseZ(Z, consumeToken()) || !Z)
-              return true;
+              return Error(getLexer().getLoc(),
+                           "Expected a {z} mark at this point");
 
           }
           // '{z}' on its own is meaningless, hence should be ignored.
           // on the contrary - have it been accompanied by a K register,
           // allow it.
           if (Z)
             Operands.push_back(std::move(Z));
         }

test/MC/X86/avx512-err.s

-// RUN: not llvm-mc -triple x86_64-unknown-unknown -mcpu=knl -mattr=+avx512dq --show-encoding %s 2> %t.err
+// RUN: not llvm-mc -triple x86_64-unknown-unknown -mcpu=knl -mattr=+avx512dq -mattr=+avx512f --show-encoding %s 2> %t.err
 // RUN: FileCheck --check-prefix=ERR < %t.err %s
 
 // ERR: invalid operand for instruction
 vpcmpd $1, %zmm24, %zmm7, %k5{%k0}
 
+// ERR: Expected a {z} mark at this point
+vfmsub213ps %zmm8, %zmm8, %zmm8{%k2} {rn-sae}
+
+// ERR: Expected an op-mask register at this point
+vfmsub213ps %zmm8, %zmm8, %zmm8 {rn-sae}