Page MenuHomePhabricator

Adding altmacro support in integrated assembler.
AbandonedPublic

Authored by m_zuckerman on Jun 21 2015, 12:04 AM.

Details

Summary

The asm parser now support the altmacro and noaltmacro directives.
This is flow the bug in link: https://llvm.org/bugs/show_bug.cgi?id=18918
You can find information about altmacro in this link :https://sourceware.org/binutils/docs/as/Altmacro.html.
In this part of the work we support the three type of string "..." , '...' ,<...>.
You can now use '%' char to write `%expr’ to evaluate the expression expr and use the result as a string.
In the future we will add the local directive and additional string support.

Diff Detail

Event Timeline

m_zuckerman retitled this revision from to Adding altmacro support in integrated assembler..
m_zuckerman updated this object.
m_zuckerman edited the test plan for this revision. (Show Details)
rafael added a subscriber: Unknown Object (MLST).Jun 22 2015, 7:52 AM
rafael added inline comments.Jun 22 2015, 8:02 AM
include/llvm/MC/MCParser/AsmLexer.h
58

Variable name starts with uppercase.

73

Function name starts with a lowercase. You can just update the existing ones in a cleanup patch first if you want.

lib/MC/MCParser/AsmLexer.cpp
367

Handel?

419

Using LastChar you can have a single ReturnError, no?

498–631

Don't repeat name in comment.

This description should probably be at the declaration.

582

Please commit the format only changes first and rebase.

lib/MC/MCParser/AsmParser.cpp
1242

The old formatting looks like the more common one.

m_zuckerman added inline comments.Jun 23 2015, 4:21 AM
include/llvm/MC/MCParser/AsmLexer.h
58

accept

73

accept

lib/MC/MCParser/AsmLexer.cpp
367

accept
was spelling mistake handel -> handle

419

accept

498–631

accept

582

accept

lib/MC/MCParser/AsmParser.cpp
1242

accept

this is the update file after first review

there is some not my changes from base.
use compere to see the new patch(diff2) with the diff1
and diff1 with base
thanks

rafael edited edge metadata.Jun 23 2015, 8:14 AM

I tried applying the patch on current trunk but it fails. Please rebase.

include/llvm/MC/MCParser/AsmLexer.h
58

Don't repeat name in comment.

lib/MC/MCParser/AsmLexer.cpp
427

What is this for? Is <> delimited strings only valid if followed by , or newline?!

rebase in :
This is the new review: http://reviews.llvm.org/D10692

include/llvm/MC/MCParser/AsmLexer.h
58

accept

lib/MC/MCParser/AsmLexer.cpp
427

for the <>I use the same output as the "" of llvm use empty string
example:
run: llvm-mc filename.asm
.altmacro
.macro mksym, name, number

.long \name\number

.long \name
.endm
mksym "",1
mksym <>,2
output
.text

.long 1

.long 2

compnerd edited edge metadata.Jul 22 2015, 7:52 PM

This seems extremely under-tested. I remember looking into this, and holding off on the implementation, since it needed some reworking of the expression evaluation, since there are differences in the different modes.

lib/MC/MCParser/AsmLexer.cpp
502

Space before the '{'.

556

Space around the 'else'

604

Why leave LexQuote in a comment?

609

Space after the colon

619

Space after colon

lib/MC/MCParser/AsmParser.cpp
180

Variables start with a CapitalLetter.

250

Partial Redundancy Elimination?

421–423

The comment is unnecessary and misleading.

799

Isn't this equivalent to:

case AsmToken::Percent:
  return !AltMacroMode;
1241

unnecessary whitespace change

m_zuckerman abandoned this revision.Jul 23 2015, 1:24 AM