[x86][inline-asm][avx512] allow swapping of '{k<num>}' & '{z}' marks

[coby]

Summary

Desc:

AVX512 allows dest operand to be followed by an op-mask register specifier ('{k<num>}', which in turn may be followed by a merging/zeroing specifier ('{z}')
Currently, the following forms are allowed:
{k<num>}
{k<num>}{z}

This patch allows the following forms:
{z}{k<num>}

and ignores the next form:
{z}

Justification would be quite simple - GCC

[delena]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1887 ↗

(On Diff #72794)

Please add more comments about functionality.

1888 ↗

(On Diff #72794)

I suggest to remove IsExpected. Just look for Z and return true if found.

1908 ↗

(On Diff #72794)

Comments about what this function does.

1909 ↗

(On Diff #72794)

I suggest to return found/not-found. Looks like you continue parsing after error.

1914 ↗

(On Diff #72794)

can you use "auto" instead of std::unique_ptr<X86Operand> ?

1916 ↗

(On Diff #72794)

Why return !Error() ??
Should we assume that Error() returns true?

[coby]

Simplification

[rnk]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1884 ↗

(On Diff #72993)

Please use the asmparser the convention that 'true' represents an error condition. I'm aware that it's surprising, but I want the code to be consistent until someone decides to do a global refactoring.

1897–1899 ↗

(On Diff #72993)

This can be simplified to:

if (parseToken(AsmToken::RCurly, "expected } at this point"))
  return true;

1908 ↗

(On Diff #72794)

Oh dear, this is opposite from the convention of the rest of the asmparser. This has exactly one call site and it is local to this file. Do you mind flipping the convention to match the rest of this file and all other asm parsers?

[coby]

@rnk: Then you suggest i'll flip HandleAVX512Opernad as well? As it is not adhering the convention as well

[rnk]

In D25013#562032, @coby wrote:

@rnk: Then you suggest i'll flip HandleAVX512Opernad as well? As it is not adhering the convention as well

Yes please. It looks like a small change.

[coby]

Flipping boolean return value(s) so proposed changes will be on par with current convention.
affected subroutines:

ParseZ
HandleAVX512Operand

[rnk]

looks good, but I'd recommend using the parse* helpers.

lib/Target/X86/AsmParser/X86AsmParser.cpp

1933–1935 ↗

(On Diff #73810)

This is also equivalent to parseToken

1897–1899 ↗

(On Diff #72993)

Any reason not to do this?

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1897–1899 ↗

(On Diff #72993)

Some.
One would be conformity, the second will apply to readability and the other is a somewhat of a flavor issue:

1. While 'parseToken' might (or might not, haven't really checked) be considered as a common way of reporting errors - it is no where to be seen on the currently reviewed source file. hence, at least until global re-factorization is at hand - I would tend to stick to the old and (possibly) good.
2. the title 'parseToken' does brilliant job stating its role as a Parsing subroutine, but fails to reveal its part as an error-reporting mechanism.
3. imho, and as a side note, the entire error reporting handling is a bit of lacky, mainly the need to dictate an error message each time, but it is a subject for another discussion..

llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp

bool MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
                                     OperandVector &Operands, MCStreamer &Out,
                                     uint64_t &ErrorInfo,
                                     bool MatchingInlineAsm);
 
   bool OmitRegisterFromClobberLists(unsigned RegNo) override;
 
   /// Parses AVX512 specific operand primitives: masked registers ({%k<NUM>}, {z})
   /// and memory broadcasting ({1to<NUM>}) primitives, updating Operands vector if required.
-  /// \return \c true if no parsing errors occurred, \c false otherwise.
+  /// return false if no parsing errors occurred, true otherwise.
   bool HandleAVX512Operand(OperandVector &Operands,
                            const MCParsedAsmOperand &Op);
 
+  bool ParseZ(std::unique_ptr<X86Operand> &Z, const SMLoc &StartLoc);
+
   bool is64BitMode() const {
     // FIXME: Can tablegen auto-generate this?
     return getSTI().getFeatureBits()[X86::Mode64Bit];
   }
   bool is32BitMode() const {
     // FIXME: Can tablegen auto-generate this?
     return getSTI().getFeatureBits()[X86::Mode32Bit];
   }
case AsmToken::LCurly:{
     SMLoc Start = Parser.getTok().getLoc(), End;
     if (getSTI().getFeatureBits()[X86::FeatureAVX512])
       return ParseRoundingModeOp(Start, End);
     return ErrorOperand(Start, "Unexpected '{' in expression");
   }
   }
 }
 
+// true on failure, false otherwise
+// If no {z} mark was found - Parser doesn't advance
+bool X86AsmParser::ParseZ(std::unique_ptr<X86Operand> &Z,
+                          const SMLoc &StartLoc) {
+  MCAsmParser &Parser = getParser();
+  // Assuming we are just pass the '{' mark, quering the next token
+  // Searched for {z}, but none was found. Return true, as no parsing error was
+  // encountered
+  if (!(getLexer().is(AsmToken::Identifier) &&
+        (getLexer().getTok().getIdentifier() == "z")))
+    return false;
+  Parser.Lex(); // Eat z
+  // Query and eat the '}' mark
+  if (!getLexer().is(AsmToken::RCurly))
+    return Error(getLexer().getLoc(), "Expected } at this point");
+  Parser.Lex(); // Eat '}'
+  // Assign Z with the {z} mark opernad
+  Z.reset(X86Operand::CreateToken("{z}", StartLoc).release());
+  return false;
+}
+
+// true on failure, false otherwise
 bool X86AsmParser::HandleAVX512Operand(OperandVector &Operands,
                                        const MCParsedAsmOperand &Op) {
   MCAsmParser &Parser = getParser();
   if(getSTI().getFeatureBits()[X86::FeatureAVX512]) {
     if (getLexer().is(AsmToken::LCurly)) {
       // Eat "{" and mark the current place.
       const SMLoc consumedToken = consumeToken();
       // Distinguish {1to<NUM>} from {%k<NUM>}.
       if(getLexer().is(AsmToken::Integer)) {
         // Parse memory broadcasting ({1to<NUM>}).
         if (getLexer().getTok().getIntVal() != 1)
-          return !TokError("Expected 1to<NUM> at this point");
+          return TokError("Expected 1to<NUM> at this point");
         Parser.Lex();  // Eat "1" of 1to8
         if (!getLexer().is(AsmToken::Identifier) ||
             !getLexer().getTok().getIdentifier().startswith("to"))
-          return !TokError("Expected 1to<NUM> at this point");
+          return TokError("Expected 1to<NUM> at this point");
         // Recognize only reasonable suffixes.
         const char *BroadcastPrimitive =
           StringSwitch<const char*>(getLexer().getTok().getIdentifier())
             .Case("to2",  "{1to2}")
             .Case("to4",  "{1to4}")
             .Case("to8",  "{1to8}")
             .Case("to16", "{1to16}")
             .Default(nullptr);
         if (!BroadcastPrimitive)
-          return !TokError("Invalid memory broadcast primitive.");
+          return TokError("Invalid memory broadcast primitive.");
         Parser.Lex();  // Eat "toN" of 1toN
         if (!getLexer().is(AsmToken::RCurly))
-          return !TokError("Expected } at this point");
+          return TokError("Expected } at this point");
         Parser.Lex();  // Eat "}"
         Operands.push_back(X86Operand::CreateToken(BroadcastPrimitive,
                                                    consumedToken));
         // No AVX512 specific primitives can pass
         // after memory broadcasting, so return.
-        return true;
+        return false;
       } else {
-        // Parse mask register {%k1}
-        Operands.push_back(X86Operand::CreateToken("{", consumedToken));
+        // Parse either {k}{z}, {z}{k}, {k} or {z}
+        // last one have no meaning, but GCC accepts it
+        // 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;
+          // Parse an op-mask register mark ({%k<NUM>}), which is now to be
+          // expected
           if (std::unique_ptr<X86Operand> Op = ParseOperand()) {
-          Operands.push_back(std::move(Op));
             if (!getLexer().is(AsmToken::RCurly))
-            return !TokError("Expected } at this point");
+              return Error(getLexer().getLoc(), "Expected } at this point");
+            Operands.push_back(X86Operand::CreateToken("{", StartLoc));
+            Operands.push_back(std::move(Op));
             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;
 
-          // Parse "zeroing non-masked" semantic {z}
-          if (getLexer().is(AsmToken::LCurly)) {
-            Operands.push_back(X86Operand::CreateToken("{z}", consumeToken()));
-            if (!getLexer().is(AsmToken::Identifier) ||
-                getLexer().getTok().getIdentifier() != "z")
-              return !TokError("Expected z at this point");
-            Parser.Lex();  // Eat the z
-            if (!getLexer().is(AsmToken::RCurly))
-              return !TokError("Expected } at this point");
-            Parser.Lex();  // Eat the }
           }
+          // '{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));
         }
       }
     }
   }
-  return true;
+  return false;
 }
 
 /// ParseMemOperand: segment: disp(basereg, indexreg, scale).  The '%ds:' prefix
 /// has already been parsed if present.
 std::unique_ptr<X86Operand> X86AsmParser::ParseMemOperand(unsigned SegReg,
                                                           SMLoc MemStart) {
 
   MCAsmParser &Parser = getParser();
if (getLexer().isNot(AsmToken::EndOfStatement) && !isPrefix) {
     // Parse '*' modifier.
     if (getLexer().is(AsmToken::Star))
       Operands.push_back(X86Operand::CreateToken("*", consumeToken()));
 
     // Read the operands.
     while(1) {
       if (std::unique_ptr<X86Operand> Op = ParseOperand()) {
         Operands.push_back(std::move(Op));
-        if (!HandleAVX512Operand(Operands, *Operands.back()))
+        if (HandleAVX512Operand(Operands, *Operands.back()))
           return true;
       } else {
          return true;
       }
       // check for comma and eat it
       if (getLexer().is(AsmToken::Comma))
         Parser.Lex();
       else

llvm/trunk/test/MC/X86/avx512bw-encoding.s

 // RUN: llvm-mc -triple x86_64-unknown-unknown -mcpu=skx  --show-encoding %s | FileCheck %s
 
 // CHECK: vpblendmb %zmm25, %zmm18, %zmm17
 // CHECK:  encoding: [0x62,0x82,0x6d,0x40,0x66,0xc9]
           vpblendmb %zmm25, %zmm18, %zmm17
 
 // CHECK: vpblendmb %zmm25, %zmm18, %zmm17 {%k5}
 // CHECK:  encoding: [0x62,0x82,0x6d,0x45,0x66,0xc9]
           vpblendmb %zmm25, %zmm18, %zmm17 {%k5}
 
 // CHECK: vpblendmb %zmm25, %zmm18, %zmm17 {%k5} {z}
 // CHECK:  encoding: [0x62,0x82,0x6d,0xc5,0x66,0xc9]
           vpblendmb %zmm25, %zmm18, %zmm17 {%k5} {z}
 
+// CHECK: vpblendmb %zmm25, %zmm18, %zmm17 {%k5} {z}
+// CHECK:  encoding: [0x62,0x82,0x6d,0xc5,0x66,0xc9]
+          vpblendmb %zmm25, %zmm18, %zmm17 {z} {%k5}
+
 // CHECK: vpblendmb (%rcx), %zmm18, %zmm17
 // CHECK:  encoding: [0x62,0xe2,0x6d,0x40,0x66,0x09]
           vpblendmb (%rcx), %zmm18, %zmm17
 
 // CHECK: vpblendmb 291(%rax,%r14,8), %zmm18, %zmm17
 // CHECK:  encoding: [0x62,0xa2,0x6d,0x40,0x66,0x8c,0xf0,0x23,0x01,0x00,0x00]
           vpblendmb 291(%rax,%r14,8), %zmm18, %zmm17
 

llvm/trunk/test/MC/X86/intel-syntax-avx512.s

 // CHECK: vaddpd  zmm1 {k5}, zmm1, zmm2
 // CHECK:  encoding: [0x62,0xf1,0xf5,0x4d,0x58,0xca]
 vaddpd zmm1 {k5}, zmm1, zmm2
 
 // CHECK: vaddpd zmm1 {k5} {z}, zmm1, zmm2
 // CHECK:  encoding: [0x62,0xf1,0xf5,0xcd,0x58,0xca]
 vaddpd zmm1 {k5} {z}, zmm1, zmm2
 
+// CHECK: vaddpd zmm1 {k5} {z}, zmm1, zmm2
+// CHECK:  encoding: [0x62,0xf1,0xf5,0xcd,0x58,0xca]
+vaddpd zmm1 {z} {k5}, zmm1, zmm2
+
 // CHECK: vaddpd zmm1, zmm1, zmm2, {rn-sae}
 // CHECK:  encoding: [0x62,0xf1,0xf5,0x18,0x58,0xca]
 vaddpd zmm1, zmm1, zmm2, {rn-sae}
 
 // CHECK: vaddpd zmm1, zmm1, zmm2, {ru-sae}
 // CHECK:  encoding: [0x62,0xf1,0xf5,0x58,0x58,0xca]
 vaddpd zmm1, zmm1, zmm2, {ru-sae}