This is an archive of the discontinued LLVM Phabricator instance.

[AsmParser][SystemZ][z/OS] Add support to AsmLexer to accept HLASM style integers
ClosedPublic

Authored by anirudhp on Mar 25 2021, 1:19 PM.

Details

Summary
  • Add support for HLASM style integers. These are the decimal integers [0-9].
  • HLASM does not support the additional prefixed integers like, 0b, 0x, octal integers and Masm style integers.
  • To achieve this, a field LexHLASMStyleIntegers (similar to the LexMasmStyleIntegers field) is introduced in MCAsmLexer.h as well as a corresponding setter.

Note: This field could also go into MCAsmInfo.h. I used the previous precedent set by the LexMasmIntegers field.

Depends on https://reviews.llvm.org/D99286

Diff Detail

Event Timeline

anirudhp created this revision.Mar 25 2021, 1:19 PM
anirudhp requested review of this revision.Mar 25 2021, 1:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2021, 1:19 PM
anirudhp edited the summary of this revision. (Show Details)Mar 25 2021, 1:21 PM

I got some very minor nits about formatting, but overall it looks good

llvm/lib/MC/MCParser/AsmLexer.cpp
325

nit: I think we should stick to either HLASM-style or HLASM-flavour for consistency and not both.

331

nit: comment style should be like the following to be clang-tidy friendly https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
389

extra space here

anirudhp updated this revision to Diff 333867.Mar 29 2021, 7:39 AM
  • Addressed comments (Minor formatting changes)
anirudhp marked 3 inline comments as done.Mar 29 2021, 7:39 AM
anirudhp edited the summary of this revision. (Show Details)Mar 31 2021, 11:09 AM
anirudhp added reviewers: thakis, MaskRay.
anirudhp updated this revision to Diff 334824.Apr 1 2021, 2:06 PM
  • Rebase on latest master
epastor requested changes to this revision.Apr 1 2021, 2:56 PM
epastor added inline comments.
llvm/lib/MC/MCParser/AsmLexer.cpp
329–330

Can we save a lot of code here by removing this whole segment and making the decimal parsing below work even with leading 0 when LexHLASMIntegers is set?

Right now, this seems very redundant.

This revision now requires changes to proceed.Apr 1 2021, 2:56 PM
anirudhp updated this revision to Diff 335264.Apr 5 2021, 8:42 AM
  • Addressed Comments
  • Moved logic for LexHLASMIntegers to the main block where decimal integers are parsed.
anirudhp marked an inline comment as done.Apr 5 2021, 8:43 AM
anirudhp added inline comments.
llvm/lib/MC/MCParser/AsmLexer.cpp
329–330

Done! Thanks for the hint!

anirudhp marked an inline comment as done.Apr 5 2021, 8:44 AM
epastor added inline comments.Apr 5 2021, 8:58 AM
llvm/lib/MC/MCParser/AsmLexer.cpp
462

Should this be (LexHLASMIntegers || CurPtr[-1] != 0) || CurPtr[0] == '.'? The parentheses may be relevant here, since you probably still want floating point literals starting or ending with '.' to be handled.

epastor accepted this revision.Apr 5 2021, 9:03 AM
epastor added inline comments.
llvm/lib/MC/MCParser/AsmLexer.cpp
462

I withdraw this comment... I think I thought the latter || was an &&. Apologies!

This revision is now accepted and ready to land.Apr 5 2021, 9:03 AM
anirudhp updated this revision to Diff 335276.Apr 5 2021, 9:25 AM
anirudhp marked an inline comment as done.
  • Appeasing clang-tidy (rename of variable from camelCase to CamelCase)
anirudhp updated this revision to Diff 337167.Apr 13 2021, 8:37 AM
anirudhp set the repository for this revision to rG LLVM Github Monorepo.
  • Rebase on latest master
anirudhp updated this revision to Diff 337225.Apr 13 2021, 12:02 PM
  • Rebase on latest master x2 (since the macho tests seem to be fixed)
This revision was landed with ongoing or failed builds.Apr 13 2021, 12:29 PM
This revision was automatically updated to reflect the committed changes.