[AsmParser][SystemZ][z/OS] Add in support to accept "#" as part of an Identifier token

[anirudhp]

Summary

• This patch adds in support to accept the "#" character as part of an Identifier.
• This support is needed especially for the HLASM dialect since "#" is treated as part of the valid "Alphabet" range
• The way this is done is by making use of the previous precedent set by the AllowAtInIdentifier field in MCAsmLexer.h. A new field called AllowHashInIdentifier is introduced.
• The static function IsIdentifierChar is also updated to accept the # character if the AllowHashInIdentifier field is set to true.

Note: The field introduced in MCAsmLexer.h could very well be moved to MCAsmInfo.h. I'm not opposed to it. I decided to put it in MCAsmLexer since there seems to be some sort of precedent already with AllowAtInIdentifier.

[abhina.sreeskantharajan]

llvm/lib/MC/MCParser/AsmLexer.cpp

145–146

nit: This comment should probably be updated to reflect the new changes.

730

same suggestion here as well. This comment may need to be updated.

[anirudhp]

• Addressing Comments (Changing comment with the updated grammar)

[anirudhp]

llvm/lib/MC/MCParser/AsmLexer.cpp

145–146

Good catch. Done.

[anirudhp]

• Addressed clang-tidy warnings/errors

[abhina.sreeskantharajan]

There is one failing mlir unit test which I suspect might be caused by this patch and should be fixed.

[anirudhp]

In D99277#2655519, @abhina.sreeskantharajan wrote:

There is one failing mlir unit test which I suspect might be caused by this patch and should be fixed.

Its probably these 2 commits and not because of my commit: https://reviews.llvm.org/rGf6e0fc2ddd8e9286e52a9931c833a64366a1ba41 and https://reviews.llvm.org/rGbc888a0fd61aa3a34102593867f76b5600c7a61e. It was fixed recently. I might have to rebase on latest master just to be consistent.

[abhina.sreeskantharajan]

In D99277#2655770, @anirudhp wrote:

In D99277#2655519, @abhina.sreeskantharajan wrote:

There is one failing mlir unit test which I suspect might be caused by this patch and should be fixed.

Its probably these 2 commits and not because of my commit: https://reviews.llvm.org/rGf6e0fc2ddd8e9286e52a9931c833a64366a1ba41 and https://reviews.llvm.org/rGbc888a0fd61aa3a34102593867f76b5600c7a61e. It was fixed recently. I might have to rebase on latest master just to be consistent.

oh you're right, those are more likely to be the cause. It would be good to rebase and double-check. The changes LGTM

[anirudhp]

• Rebasing on latest master

[anirudhp]

Gentle Ping :)

[nickdesaulniers]

llvm/include/llvm/MC/MCParser/MCAsmLexer.h

151

The only caller is a test?

I think if you add an llvm/test/MC/SystemZ style test, you'd find this code probably isn't working as expected.

It's curious to me why this differs from how AllowAtInIdentifier is set in the constructor for AsmLexer.

llvm/lib/MC/MCParser/AsmLexer.cpp

149

If you flip the order of AllowHash && C == '#' that would be better, since AllowHash is mostly always false for the general case. Feel free to reorder AllowAt and move both of those to be the final checks.

733

do we need a similar method to MCAsmInfo::doesAllowAtInName? I wonder why MCAsmInfo and MCAsmLexer have their own fields for that? Seems like perhaps a (pre-existing) mistake.

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp

98

it's not clear to me how any of the changes to llvm/lib/ affect these changes to llvm/unittests/ in regards to explicitly calling Lex.

[anirudhp]

llvm/include/llvm/MC/MCParser/MCAsmLexer.h

151

The only caller is a test?

For now, yes. There is another patch in waiting (see https://reviews.llvm.org/D98276). Ideally, the call to the setter would be set in the constructor for the HLASMAsmParser class)

I think if you add an llvm/test/MC/SystemZ style test, you'd find this code probably isn't working as expected.

A bit of background:
I probably should have mentioned this in the description, but these changes would be used by the z/OS target for which we cannot currently generate code yet (work for this is ongoing). Hence, the non-existence of a lit test (which would make things very easy for these types of changes).

Since the LLVM community prefers to have a test for any functional code changes, I came up with a unit test in the interim, since this would help "alleviate" some of the wait time (i.e. while we wait for the corresponding ObjectFile/Streamer support for the z/OS target and other patches that might be in review)

To answer your question:
Yes, a test in llvm/test/MC/SystemZ would not work as "intended" because the call to the setter isn't made anywhere. To be more specific, for the s390x-ibm-linux triple, the behaviour would be "unexpected". For an s390x-none-zos triple, the test wouldn't even run because we don't have relevant ObjectFile/Streamer support yet (as mentioned above)

It's curious to me why this differs from how AllowAtInIdentifier is set in the constructor for AsmLexer.

It doesn't differ in terms of the behaviour that it is exposing. I don't really see an issue with moving it to a constructor and providing with similar behaviour (i.e. is MAI's CommentString isn't # set it to true), but since a default value of false was provided at the time of declaration, I didn't see the need for it.

If it is preferred to move to the constructor, I don't mind doing so.

llvm/lib/MC/MCParser/AsmLexer.cpp

733

I do think you're right, I don't see a reason why for the MS-case, especially in the lexing phase, that we should be using the MCAsmInfo::doesAllowAtInName field. I did spend some time thinking about this. To me, it does look like a "pre-existing mistake" in this particular case, but I will admit I am not too familiar with the MS asm semantics.

However, in terms of the behaviour of the two fields:

From the comment in MCAsmInfo.h, it was to "allow @ in a symbol name" and this is used in AsmParser.cpp. You could have a symbol name such as "abc@variant", where "variant" could possibly be a valid symbol variant (MCSymbolRefExpr::VariantKind). If this attribute is set to true, its assumed there is no special symbol variant for it to lookup.

AllowAtInName in MCAsmLexer.h seems exclusively for lexing purposes. Where you want to include "accepting" @ when you see it as part of an AsmToken::Identifier

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp

98

My apologies. Could you expand a bit on your comment?

Afaik, we need the initial call to Lex to start the Lexer (to start getting the tokens one by one for later processing and checking). I could technically move it into the lexAndCheckTokens function before I run the loop, if that's what you mean?

But the way it was initially setup, I wanted to ensure the flag was set before the Lexer was run, and not during the checking phase, since at that point, the tokens are already Lexed, and we're merely checking, and I couldn't do that if it was encapsulated into a function that was run prior to the call to the setter.

[nickdesaulniers]

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp

98

Ah, sorry, I see now that the call to Lex was moved out of the setup code. Yeah that's fine.

[MaskRay]

llvm/include/llvm/MC/MCParser/MCAsmLexer.h

151

Since it will be immediately used and having the utility functions in this patch seem more appropriate, looks good.

[anirudhp]

• Addressed Comments x2 (Reordered the check in isIdentifierChar)

llvm/include/llvm/MC/MCParser/MCAsmLexer.h

class MCAsmLexer {
   /// The location and description of the current error
   SMLoc ErrLoc;
   std::string Err;
 
 protected: // Can only create subclasses.
   const char *TokStart = nullptr;
   bool SkipSpace = true;
   bool AllowAtInIdentifier;
+  bool AllowHashInIdentifier = false;
   bool IsAtStartOfStatement = true;
   bool LexMasmHexFloats = false;
   bool LexMasmIntegers = false;
   bool LexMasmStrings = false;
   bool UseMasmDefaultRadix = false;
   unsigned DefaultRadix = 10;
   AsmCommentConsumer *CommentConsumer = nullptr;
 
public:
   bool isNot(AsmToken::TokenKind K) const { return getTok().isNot(K); }
 
   /// Set whether spaces should be ignored by the lexer
   void setSkipSpace(bool val) { SkipSpace = val; }
 
   bool getAllowAtInIdentifier() { return AllowAtInIdentifier; }
   void setAllowAtInIdentifier(bool v) { AllowAtInIdentifier = v; }
 
+  void setAllowHashInIdentifier(bool V) { AllowHashInIdentifier = V; }
+
   void setCommentConsumer(AsmCommentConsumer *CommentConsumer) {
     this->CommentConsumer = CommentConsumer;
   }
 
   /// Set whether to lex masm-style binary (e.g., 0b1101) and radix-specified
   /// literals (e.g., 0ABCh [hex], 576t [decimal], 77o [octal], 1101y [binary]).
   void setLexMasmIntegers(bool V) { LexMasmIntegers = V; }
 

llvm/lib/MC/MCParser/AsmLexer.cpp

while (isDigit(*CurPtr))
     ++CurPtr;
 
   if (CurPtr == ExpStart)
     return ReturnError(TokStart, "invalid hexadecimal floating-point constant: "
                                  "expected at least one exponent digit");
 
   return AsmToken(AsmToken::Real, StringRef(TokStart, CurPtr - TokStart));
 }
 
-/// LexIdentifier: [a-zA-Z_.][a-zA-Z0-9_$.@?]*
-static bool IsIdentifierChar(char c, bool AllowAt) {
-  return isAlnum(c) || c == '_' || c == '$' || c == '.' ||
-         (c == '@' && AllowAt) || c == '?';
+/// LexIdentifier: [a-zA-Z_.][a-zA-Z0-9_$.@#?]*
+static bool isIdentifierChar(char C, bool AllowAt, bool AllowHash) {
+  return isAlnum(C) || C == '_' || C == '$' || C == '.' || C == '?' ||
+         (AllowAt && C == '@') || (AllowHash && C == '#');
 }
 
 AsmToken AsmLexer::LexIdentifier() {
   // Check for floating point literals.
   if (CurPtr[-1] == '.' && isDigit(*CurPtr)) {
     // Disambiguate a .1243foo identifier from a floating literal.
     while (isDigit(*CurPtr))
       ++CurPtr;
 
-    if (!IsIdentifierChar(*CurPtr, AllowAtInIdentifier) ||
+    if (!isIdentifierChar(*CurPtr, AllowAtInIdentifier,
+                          AllowHashInIdentifier) ||
         *CurPtr == 'e' || *CurPtr == 'E')
       return LexFloatLiteral();
   }
 
-  while (IsIdentifierChar(*CurPtr, AllowAtInIdentifier))
+  while (isIdentifierChar(*CurPtr, AllowAtInIdentifier, AllowHashInIdentifier))
     ++CurPtr;
 
   // Handle . as a special case.
   if (CurPtr == TokStart+1 && TokStart[0] == '.')
     return AsmToken(AsmToken::Dot, StringRef(TokStart, 1));
 
   return AsmToken(AsmToken::Identifier, StringRef(TokStart, CurPtr - TokStart));
 }
if (CurChar == EOF && !IsAtStartOfStatement && EndStatementAtEOF) {
     return AsmToken(AsmToken::EndOfStatement, StringRef(TokStart, 0));
   }
   IsAtStartOfLine = false;
   bool OldIsAtStartOfStatement = IsAtStartOfStatement;
   IsAtStartOfStatement = false;
   switch (CurChar) {
   default:
     if (MAI.doesAllowSymbolAtNameStart()) {
-      // Handle Microsoft-style identifier: [a-zA-Z_$.@?][a-zA-Z0-9_$.@?]*
+      // Handle Microsoft-style identifier: [a-zA-Z_$.@?][a-zA-Z0-9_$.@#?]*
       if (!isDigit(CurChar) &&
-          IsIdentifierChar(CurChar, MAI.doesAllowAtInName()))
+          isIdentifierChar(CurChar, MAI.doesAllowAtInName(),
+                           AllowHashInIdentifier))
         return LexIdentifier();
     } else {
       // Handle identifier: [a-zA-Z_.][a-zA-Z0-9_$.@]*
       if (isalpha(CurChar) || CurChar == '_' || CurChar == '.')
         return LexIdentifier();
     }
 
     // Unknown character, emit an error.

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp

void setupCallToAsmParser(StringRef AsmStr) {
 
     Ctx.reset(
         new MCContext(MUPMAI.get(), MRI.get(), &MOFI, &SrcMgr, &MCOptions));
     MOFI.InitMCObjectFileInfo(Triple, false, *Ctx, false);
 
     Str.reset(TheTarget->createNullStreamer(*Ctx));
 
     Parser.reset(createMCAsmParser(SrcMgr, *Ctx, *Str, *MUPMAI));
-    // Lex initially to get the string.
-    Parser->getLexer().Lex();
   }
 
   void lexAndCheckTokens(StringRef AsmStr,
                          SmallVector<AsmToken::TokenKind> ExpectedTokens) {
     // Get reference to AsmLexer.
     MCAsmLexer &Lexer = Parser->getLexer();
     // Loop through all expected tokens checking one by one.
     for (size_t I = 0; I < ExpectedTokens.size(); ++I) {
       EXPECT_EQ(Lexer.getTok().getKind(), ExpectedTokens[I]);
       Lexer.Lex();
     }
   }
 };
 
 TEST_F(SystemZAsmLexerTest, CheckDontRestrictCommentStringToStartOfStatement) {
   StringRef AsmStr = "jne #-4";
 
   // Setup.
   setupCallToAsmParser(AsmStr);
 
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
   SmallVector<AsmToken::TokenKind> ExpectedTokens(
       {AsmToken::Identifier, AsmToken::EndOfStatement});
   lexAndCheckTokens(AsmStr /* "jne #-4" */, ExpectedTokens);
 }
 
 // Testing MCAsmInfo's RestrictCommentStringToStartOfStatement attribute.
 TEST_F(SystemZAsmLexerTest, CheckRestrictCommentStringToStartOfStatement) {
   StringRef AsmStr = "jne #-4";
 
   // Setup.
   MUPMAI->setRestrictCommentStringToStartOfStatement(true);
   setupCallToAsmParser(AsmStr);
 
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
   // When we are restricting the comment string to only the start of the
   // statement, The sequence of tokens we are expecting are: Identifier - "jne"
   // Hash - '#'
   // Minus - '-'
   // Integer - '4'
   SmallVector<AsmToken::TokenKind> ExpectedTokens(
       {AsmToken::Identifier, AsmToken::Hash, AsmToken::Minus,
        AsmToken::Integer});
   lexAndCheckTokens(AsmStr /* "jne #-4" */, ExpectedTokens);
 }
 
 // Test HLASM Comment Syntax ('*')
 TEST_F(SystemZAsmLexerTest, CheckHLASMComment) {
   StringRef AsmStr = "* lhi 1,10";
 
   // Setup.
   MUPMAI->setCommentString("*");
   setupCallToAsmParser(AsmStr);
 
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
   SmallVector<AsmToken::TokenKind> ExpectedTokens(
       {AsmToken::EndOfStatement, AsmToken::Eof});
   lexAndCheckTokens(AsmStr /* "* lhi 1,10" */, ExpectedTokens);
 }
+
+TEST_F(SystemZAsmLexerTest, CheckHashDefault) {
+  StringRef AsmStr = "lh#123";
+
+  // Setup.
+  setupCallToAsmParser(AsmStr);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  // "lh" -> Identifier
+  // "#123" -> EndOfStatement (Lexed as a comment since CommentString is "#")
+  SmallVector<AsmToken::TokenKind> ExpectedTokens(
+      {AsmToken::Identifier, AsmToken::EndOfStatement, AsmToken::Eof});
+  lexAndCheckTokens(AsmStr, ExpectedTokens);
+}
+
+// Test if "#" is accepted as an Identifier
+TEST_F(SystemZAsmLexerTest, CheckAllowHashInIdentifier) {
+  StringRef AsmStr = "lh#123";
+
+  // Setup.
+  setupCallToAsmParser(AsmStr);
+  Parser->getLexer().setAllowHashInIdentifier(true);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  // "lh123" -> Identifier
+  SmallVector<AsmToken::TokenKind> ExpectedTokens(
+      {AsmToken::Identifier, AsmToken::EndOfStatement, AsmToken::Eof});
+  lexAndCheckTokens(AsmStr, ExpectedTokens);
+}
+
+TEST_F(SystemZAsmLexerTest, CheckAllowHashInIdentifier2) {
+  StringRef AsmStr = "lh#12*3";
+
+  // Setup.
+  MUPMAI->setCommentString("*");
+  MUPMAI->setRestrictCommentStringToStartOfStatement(true);
+  setupCallToAsmParser(AsmStr);
+  Parser->getLexer().setAllowHashInIdentifier(true);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  // "lh#12" -> Identifier
+  // "*" -> Star
+  // "3" -> Integer
+  SmallVector<AsmToken::TokenKind> ExpectedTokens(
+      {AsmToken::Identifier, AsmToken::Star, AsmToken::Integer,
+       AsmToken::EndOfStatement, AsmToken::Eof});
+  lexAndCheckTokens(AsmStr, ExpectedTokens);
+}
 } // end anonymous namespace