This is an archive of the discontinued LLVM Phabricator instance.

[MC] Allowing the use of $-prefixed integer as asm identifiers
ClosedPublic

Authored by pratlucas on Feb 25 2020, 5:09 AM.

Details

Summary

Dollar signed prefixed integers were not allowed by the AsmParser to be
used as Identifiers, differing from the GNU assembler behavior.

This patch updates the parsing of Identifiers to consider such cases as
valid, where the identifier string includes the $ prefix itself. As the
Lexer currently splits these occurrences into separate tokens, those
need to be combined by the AsmParser itself.

Event Timeline

pratlucas created this revision.Feb 25 2020, 5:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2020, 5:09 AM
efriedma added inline comments.Feb 25 2020, 3:41 PM
llvm/include/llvm/MC/MCAsmMacro.h
100 ↗(On Diff #246419)

I'd prefer to write out the new logic at the location of the relevant caller, instead. (Anything can call getString().)

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

String? I don't think we want to allow $"foo".

pratlucas updated this revision to Diff 246659.Feb 26 2020, 3:10 AM

Addressing review comments.

pratlucas marked 2 inline comments as done.Feb 26 2020, 3:13 AM
jrtc27 added inline comments.Feb 26 2020, 4:17 AM
llvm/lib/MC/MCParser/AsmParser.cpp
2861

Comment is outdated

2867

Ditto

jrtc27 added inline comments.Feb 26 2020, 4:19 AM
llvm/lib/MC/MCParser/AsmParser.cpp
2867

Actually I think this one is still fine?

pratlucas updated this revision to Diff 247572.Mar 2 2020, 1:49 AM

Updating outdated comment.

pratlucas marked 4 inline comments as done.Mar 2 2020, 1:50 AM
pratlucas added inline comments.
llvm/lib/MC/MCParser/AsmParser.cpp
2867

I believe this one is still fine, as it references the identifier being parsed and not the actual token.

pratlucas updated this revision to Diff 247594.Mar 2 2020, 3:54 AM
pratlucas marked an inline comment as done.

Clang-format fixes.

This revision is now accepted and ready to land.Mar 2 2020, 12:04 PM
This revision was automatically updated to reflect the committed changes.
llvm/test/MC/ARM/arm-branches.s