Page MenuHomePhabricator

[AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1
AcceptedPublic

Authored by anirudhp on Mar 9 2021, 11:29 AM.

Details

Summary
  • This patch (is one in a series of patches) which introduces HLASM Parser support (for the first parameter of inline asm statements) to LLVM (main RFC here)
  • This patch in particular introduces HLASM Parser support for Z machine instructions.
  • The approach taken here was to subclass AsmParser, and make various functions and variables as "protected" wherever appropriate.
  • The HLASMAsmParser class overrides the parseStatement function. Two new private functions parseAsHLASMLabel and parseAsMachineInstruction are introduced as well.

The general syntax is laid out as follows (more information available in HLASM V1R6 Language Reference Manual - Chapter 2 - Instruction Statement Format):

<TokA><spaces.*><TokB><spaces.*><TokC><spaces.*><TokD>
  1. TokA is referred to as the Name Entry. This token is optional
  2. TokB is referred to as the Operation Entry. This token is mandatory.
  3. TokC is referred to as the Operand Entry. This token is mandatory
  4. TokD is referred to as the Remarks Entry. This token is optional
  • If TokA is provided, then we either parse TokA as a possible comment or as a label (Name Entry), Tok B as the Operation Entry and so on.
  • If TokA is not provided (i.e. we have one or more spaces and then the first token), then we will parse the first token (i.e TokB) as a possible Z machine instruction, TokC as the operands to the Z machine instruction and TokD as a possible Remark field
  • TokC (Operand Entry), no spaces are allowed between OperandEntries. If a space occurs it is classified as an error.
  • TokD if provided is taken as is, and emitted as a comment.

The following additional approach was examined, but not taken:

  • Adding custom private only functions to base AsmParser class, and only invoking them for z/OS. While this would eliminate the need for another child class, these private functions would be of non-use to every other target. Similarly, adding any pure virtual functions to the base MCAsmParser class and overriding them in AsmParser would also have the same disadvantage.

Testing:

  • This patch doesn't have tests added with it, for the sole reason that MCStreamer Support and Object File support hasn't been added for the z/OS target (yet). Hence, it's not possible generate code outright for the z/OS target. They are in the process of being committed / process of being worked on.
  • Any comments / feedback on how to combat this "lack of testing" due to other missing required features is appreciated.

Diff Detail

Event Timeline

anirudhp created this revision.Mar 9 2021, 11:29 AM
anirudhp requested review of this revision.Mar 9 2021, 11:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2021, 11:29 AM
anirudhp edited the summary of this revision. (Show Details)Mar 9 2021, 11:37 AM
anirudhp retitled this revision from [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser to [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1.
anirudhp edited the summary of this revision. (Show Details)Mar 9 2021, 11:40 AM
anirudhp updated this revision to Diff 329427.Mar 9 2021, 11:51 AM
anirudhp updated this revision to Diff 330073.Mar 11 2021, 2:38 PM
  • A bit of refactoring (reduced a bit of duplication of code). Removed functions which no longer need to be under "protected".
uweigand added inline comments.Mar 12 2021, 12:31 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
513

Maybe just pass MAI into the routine, so it can check those parameters itself?

llvm/lib/MC/MCParser/AsmParser.cpp
6168

I guess now this doesn't really have to be a separate routine any more?

llvm/include/llvm/MC/MCAsmInfo.h
630

nit: Move this between getCommentString and getLabelSuffix to maintain the same order the class members are listed as.

anirudhp updated this revision to Diff 330253.Mar 12 2021, 8:26 AM
  • Addressing comments
anirudhp marked 2 inline comments as done and an inline comment as not done.Mar 12 2021, 8:29 AM
anirudhp added inline comments.
llvm/include/llvm/MC/MCAsmInfo.h
630

Sure. But I'll wait until https://reviews.llvm.org/D97703 is merged in, as its around the same area. When I rebase it, I'll make sure to move it to the right place.

llvm/lib/MC/MCParser/AsmParser.cpp
6168

Yes, you're right. But since the rest of the function is essentially just getTargetParser().ParseInstruction and getTargetParser().MatchAndEmitInstruction(), I didn't want to decompose logic in checkAndGenerateLOCDirectiveForInstruction into the same function. I'd rather have it as a separate routine, since it does something different.

anirudhp marked an inline comment as done.Mar 12 2021, 8:29 AM
uweigand added inline comments.Mar 12 2021, 9:14 AM
llvm/lib/MC/MCParser/AsmParser.cpp
6168

My point was that it might be preferable to minimize changes to existing code. Just split the old parseStatement in two parts. Leave the second part (the new parseAndMatchAndEmitTargetInstruction) where it is above. The diff should just show the end of one function and the header of the new function being inserted, the rest of the lines are unchanged.

anirudhp updated this revision to Diff 330756.Mar 15 2021, 12:07 PM
  • Addressed Comments x2
  • Removing an unneeded function based on refactoring
anirudhp marked an inline comment as done.Mar 15 2021, 12:07 PM

Ping Ping! :) :)

I have two nits, but overall LGTM, your patch may need to be rebased because CI is failing

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
278

nit: Do we need to save this to a bool? We can just do the following if there is only one use.
if (MAI->getEmitGNUAsmStartIndentationMarker())

anirudhp updated this revision to Diff 335290.Apr 5 2021, 10:31 AM
  • Rebasing on latest master + setting the AllowHashInIdentifier in HLASMAsmParser since it has landed.
anirudhp marked 2 inline comments as done.Apr 5 2021, 10:31 AM

Hi All!
Does anyone have feedback on the patch / approach taken? Any opinions would be greatly welcome and appreciated! :)

Is it possible to add partial unit tests like you did in your previous patches?

Is it possible to add partial unit tests like you did in your previous patches?

No (atleast from the top of my head). The very instantiation of the Parser instance is dependent on the target triple itself. And since we don't really have other object writer/streamer support yet, there's no way to instantiate for z/OS (even in the unit test). Also, writing tests end to test the end to end implementation for the parser would be a tremendous amount of work (comparing it to writing simple mc tests).

Kai added inline comments.Wed, Apr 21, 11:26 AM
llvm/include/llvm/MC/MCAsmInfo.h
142

The comment contradicts the source below.

anirudhp updated this revision to Diff 341296.Wed, Apr 28, 1:06 PM
  • Rebased on latest to bring in new changes
  • Corrected erroneous comment
anirudhp marked an inline comment as done.Wed, Apr 28, 1:06 PM

Ping! :)

Kai accepted this revision.Fri, Apr 30, 8:54 AM

LGTM.

llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp
1386

Nit: If you combine both ifs, then it's more clearer that you just check that there is no space.

This revision is now accepted and ready to land.Fri, Apr 30, 8:54 AM
anirudhp updated this revision to Diff 342545.Mon, May 3, 2:35 PM
  • Addressing small comment regarding coalescing if statements + rebase on latest.
anirudhp updated this revision to Diff 343260.Wed, May 5, 6:23 PM
  • Rebase on latest master + update various HLASM specific setters from MCAsmLexer.