This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/AsmParser: Add support for parsing symbol operands
ClosedPublic

Authored by tstellarAMD on Jun 6 2016, 1:11 PM.

Diff Detail

Event Timeline

tstellarAMD retitled this revision from to AMDGPU/AsmParser: Add support for parsing symbol operands.
tstellarAMD updated this object.
tstellarAMD added a subscriber: llvm-commits.

Add testcase.

arsenm added a comment.Jun 6 2016, 1:23 PM

I thought symbols usually had some special prefix character to distinguish them

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
149

This looks redundant with the cast<>

1339

Typo difficlut

Remove an extra call to Lex().

tstellarAMD marked an inline comment as done.Jun 6 2016, 2:30 PM
tstellarAMD added inline comments.
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
149

It's not redundant, because Expr could be any of the MCExpr sub-classes. For example in the added tests case, Expr is sometimes an MCBinaryExpr.

vpykhtin accepted this revision.Jun 7 2016, 4:09 AM
vpykhtin edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jun 7 2016, 4:09 AM
artem.tamazov accepted this revision.Jun 7 2016, 4:17 AM
artem.tamazov edited edge metadata.

Please address comment about simple tests and go ahead.

test/MC/AMDGPU/global-expr.s
7–8

I woudl be nice to add also tests for simple absolute expressions, like

.set foo 4
s_mov_b32 s0, foo+2 // should load 6 to s0

Please address comment about simple tests and go ahead.

There was an unrelated bug with these expression, so I've added the test with D21236, which also fixes the bug.

This revision was automatically updated to reflect the committed changes.

It seems that labels in branches were not anticipated in this patch.
The quickfix is under review, http://reviews.llvm.org/D22133.

Shall we try another approach for this feature; this one looks like a hack?..
Another problem (different story, but newertheless):

.set foo 40
s_branch foo

Branch offset will be something like (40-PC) but correct one should be +40, I guess. Shall we differentiate between labels and other kinds of symbols?