[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..

AsmParser/X86AsmParser.cpp

private:
   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.
   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");
   }
   }
 }
 
+// False on failure, true 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 true;
+  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 true;
+}
+
+// False on failure, true 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::LCurly)) {
           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;
       } 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 false;
+        // 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()));
-
-          // 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 }
+          } 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 false;
+            // '{z}' on its own is meaningless, hence should be ignored.
+            // on the contrary - have it been accompanied by a K register,
+            // allow it.
+            Operands.push_back(std::move(Z));
           }
         }
       }
     }
   }
   return true;
 }