This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Override ParseDirective
ClosedPublic

Authored by xiangzhai on Sep 19 2017, 1:52 AM.

Details

Summary

Hi LLVM developers,

Motivation:

But:

$ llvm-mc -filetype=obj -triple=avr-unknown-linux -mcpu=atmega328p relax.s -o relax.o
relax.s:12:2: error: unknown directive
        .word   L2 - L1
        ^

So I simply override ParseDirective for llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp, please review it, thanks a lot!

Regards,
Leslie Zhai

Diff Detail

Repository
rL LLVM

Event Timeline

xiangzhai created this revision.Sep 19 2017, 1:52 AM
xiangzhai added a comment.EditedSep 19 2017, 2:02 AM

Dear Dylan,

BTW: AVR GNU toolchain can *not* support ldi r16, -lo8(abc) but ldi r16, lo8(-(abc)) instead,

$ avr-gcc -mmcu=atmega328p -o avr-gcc-relocations.o -c relocations.s

relocations.s: Assembler messages:
relocations.s:36: Error: garbage at end of line
relocations.s:39: Error: garbage at end of line
relocations.s:45: Error: garbage at end of line
relocations.s:51: Error: garbage at end of line
relocations.s:57: Error: garbage at end of line
relocations.s:69: Error: garbage at end of line
relocations.s:72: Error: garbage at end of line
relocations.s:75: Error: garbage at end of line
relocations.s:36: Error: can't resolve `0' {*UND* section} - `lo8' {*UND* section}
relocations.s:36: Error: expression too complex
relocations.s:39: Error: can't resolve `0' {*UND* section} - `hi8' {*UND* section}
relocations.s:39: Error: expression too complex
relocations.s:45: Error: can't resolve `0' {*UND* section} - `hh8' {*UND* section}
relocations.s:45: Error: expression too complex
relocations.s:51: Error: can't resolve `0' {*UND* section} - `hlo8' {*UND* section}
relocations.s:51: Error: expression too complex
relocations.s:57: Error: can't resolve `0' {*UND* section} - `hhi8' {*UND* section}
relocations.s:57: Error: expression too complex
relocations.s:69: Error: can't resolve `0' {*UND* section} - `pm_lo8' {*UND* section}
relocations.s:69: Error: expression too complex
relocations.s:72: Error: can't resolve `0' {*UND* section} - `pm_hi8' {*UND* section}
relocations.s:72: Error: expression too complex
relocations.s:75: Error: can't resolve `0' {*UND* section} - `pm_hh8' {*UND* section}
relocations.s:75: Error: expression too complex

So do I need to rewrite Check for sign for LLD to compatible GNU toolchain?

Regards,
Leslie Zhai

xiangzhai updated this revision to Diff 115958.Sep 19 2017, 9:13 PM

Add R_AVR_DIFF8, R_AVR_DIFF16 and R_AVR_DIFF32 testcase:

$ avr-readelf -r avr-relax.o 

Relocation section '.rela.text' at offset 0xe8 contains 1 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00000212 R_AVR_CALL        00000000   .text + 0

Relocation section '.rela.data' at offset 0xf4 contains 3 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  0000021e R_AVR_DIFF8       00000000   .text + 4
00000001  0000021f R_AVR_DIFF16      00000000   .text + 4
00000003  00000220 R_AVR_DIFF32      00000000   .text + 4
xiangzhai updated this revision to Diff 116493.Sep 24 2017, 9:53 PM
xiangzhai added reviewers: kparzysz, arsenm.

Implement Parse R_AVR_LO8_LDI_NEG and other R_AVR_XXX_NEG Expression for GCC compatible.

Because AVR GNU toolchain only support such Expression:

ldi r16, lo8(-(abc))

But LLVM AVR prefers:

ldi r16, -lo8(abc)

So I just re-implement Check for sign, please review my patch, thanks a lot!

Regards,
Leslie Zhai

Implement R_AVR_LO8_LDI_GS and R_AVR_HI8_LDI_GS:

$ llvm/build/bin/llvm-mc -filetype=obj -triple=avr-unknown-linux -mcpu=atmega328p relocations.s -o avr-llvm-relocations.o
$ avr-readelf -r avr-llvm-relocations.o

...
00000038  00000b18 R_AVR_LO8_LDI_GS  00000000   foo + 0
0000003a  00000b19 R_AVR_HI8_LDI_GS  00000000   foo + 0
...
dylanmckay added inline comments.Sep 25 2017, 7:25 PM
include/llvm/BinaryFormat/ELFRelocs/AVR.def
42

These integers correspond to numeric ELF relocation codes.

Looking at binutils-gdb, the integers for R_AVR_DIFF{8,16,32} are 30, 31, and 32 respectively.

This is concerning because 30 and 31 are already occupied by R_AVR_SYM_DIFF and R_AVR_16_LDST in master.

It looks like the AVR backend doesn't actually generate fixups for either of these, so this will have never caused any problems in the past.

If you rename SYM_DIFF to DIFF8, and then change R_AVR_16_LDST to 33, and then change your new constants to their correct values, it should be fine. I can fix up the relocations in the future.

xiangzhai updated this revision to Diff 116638.Sep 25 2017, 8:56 PM

Change RELOC_NUMBER for AVR Relocation DIFF Types.

xiangzhai marked an inline comment as done.Sep 25 2017, 8:56 PM

Implement R_AVR_16_PM:

relocations.s testcase:

...
.short foo
.short gs(foo)
...
$ avr-readelf -r avr-reloc.o 

Relocation section '.rela.text' at offset 0xa8 contains 4 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
...
00000002  00000504 R_AVR_16          00000000   foo + 0
00000004  00000505 R_AVR_16_PM       00000000   foo + 0
...
xiangzhai updated this revision to Diff 116651.EditedSep 26 2017, 2:20 AM

Does it need to Refactory ParseDirective for adding parameter OperandVector Operands to support Modifier?

xiangzhai updated this revision to Diff 116754.Sep 26 2017, 8:43 PM

Waiting for D38304 about Refactoring ParseDirective.

dylanmckay edited edge metadata.Sep 27 2017, 2:47 PM

This patch is looking really nice by the way.

lib/Target/AVR/AsmParser/AVRAsmParser.cpp
441

Can we hoist the "gs" into a static constant named `GENERATE_ at the top of the file?

It looks like gs stands for "generate stubs"

https://lists.nongnu.org/archive/html/avr-gcc-list/2010-05/msg00028.html

612

Can we do something like this at the top of the file

cpp
const int SIZE_LONG = 8;
const int SIZE_WORD = 4;

By the way, I think these sizes are for a 32-bit machine. On AVR, long is 5 bytes and int (which I think corresponds to .word) is 2 bytes

Here is a document detailing the AVR integer types and their sizes

622

Can we be more specific and name Size -> SizeInBytes

642

Replace this with

cpp
FIXME: does not support .byte lo8(foo)

Could you also add a bit more information to the comment, because it's not clear why it isn't supported.

dylanmckay added inline comments.Sep 27 2017, 2:53 PM
lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp
125

I believe we should mask out all of the upper bits here.

It looks like none of the other labels in this method do it either, which looks like a bug to me. I have filed PR34755 to fix these existing ones up.

For these new expressions, can you please mask out the irrelevant bits after shifting?

173

Related to an earlier comment; I don't think GS directly maps to program memory

Dear Dylan,

I am not available until October 10th https://mail.kde.org/pipermail/k3b/2017-September/001783.html Happy Chinese National Day!

Regards,
Leslie Zhai

xiangzhai updated this revision to Diff 118311.Oct 9 2017, 9:29 PM

Dear Dylan,

I updated my patch as your suggestion, please review it, thanks a lot!

PS: still waiting for D38304 about Refactoring ParseDirective.

Regards,
Leslie Zhai

xiangzhai marked 6 inline comments as done.Oct 9 2017, 9:30 PM
xiangzhai updated this revision to Diff 118320.EditedOct 10 2017, 12:28 AM

Supported .byte lo8(foo) now :)

$ llvm/build/bin/llvm-mc -filetype=obj -triple=avr-unknown-linux -mcpu=atmega328p relocations.s -o avr-llvm-relocations.o
$ avr-readelf -r avr-llvm-relocations.o 

Relocation section '.rela.text' at offset 0x170 contains 60 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
...
00000071  00000b1b R_AVR_8_LO8       00000000   foo + 0
...
xiangzhai updated this revision to Diff 118329.Oct 10 2017, 2:09 AM

Refactory EmitValue for AVRMCELFStreamer.

xiangzhai updated this revision to Diff 118539.Oct 10 2017, 9:49 PM

Support R_AVR_DIFF8, R_AVR_DIFF16 and R_AVR_DIFF32 now :)

$ llvm-mc -filetype=obj -triple=avr-unknown-linux -mcpu=atmega328p relax.s -o avr-relax-llvm.o

$ avr-readelf -r avr-relax-llvm.o 

Relocation section '.rela.text' at offset 0xd8 contains 1 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00000512 R_AVR_CALL        00000000   .text + 0

Relocation section '.rela.data' at offset 0xe4 contains 3 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  0000051e R_AVR_DIFF8       00000000   .text + 0
00000002  0000051f R_AVR_DIFF16      00000000   .text + 0
00000006  00000520 R_AVR_DIFF32      00000000   .text + 0
arsenm added inline comments.Oct 16 2017, 2:44 PM
lib/Target/AVR/AsmParser/AVRAsmParser.cpp
35

Generally I don't think we use these kinds of hardcoded macro options

xiangzhai updated this revision to Diff 119242.Oct 16 2017, 7:58 PM

Remove hardcoded macro REFACTORY_PARSEDIRECTIVE.

xiangzhai marked an inline comment as done.Oct 16 2017, 7:59 PM
xiangzhai updated this revision to Diff 120182.Oct 24 2017, 8:04 PM

Rebase for rL316076 and nicely Ping Dylan and Matt :)

Hey Leslie,

I don't have much time to review this in the next 3 weeks, and I also think that D38304 may take even longer to land (which this patch depends on) because it looks to have gone somewhat stale.

Dear Dylan,

Never mind, you are busy for university class :) D38304 is optional dependent, only depends on it when built with -DREFACTORY_PARSEDIRECTIVE.

Regards,
Leslie Zhai

Nicely ping Dylan :)

Have finished an initial review - change looks pretty good, it should be good to go once my comments are fixed

Sorry for taking so long to review @xiangzhai!

lib/Target/AVR/AsmParser/AVRAsmParser.cpp
14

These headers should be in alphabetical order

58

Before this can be committed, we will need to remove these REFACTORY_PARSEDIRECTIVE guards.

Once this patch has been comitted, D38304 should then be rebased

lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
270

gs stands for generate stubs - I don't think we want to apply the program memory adjustment for these. As far as I understand it, AVR::fixuo_lo8_ldi_gs is a load from RAM.

277–279

Same here regarding gs

lib/Target/AVR/MCTargetDesc/AVRELFObjectWriter.cpp
18

Place this in alphabetical order

lib/Target/AVR/MCTargetDesc/AVRMCELFStreamer.h
1 ↗(On Diff #120182)

s/MCElfStreamer/MCELFStreamer

13 ↗(On Diff #120182)

Header files should be in alphabetical order

39 ↗(On Diff #120182)

Indent this constructor list like the one above

42 ↗(On Diff #120182)

Pop an override qualifier on this function

lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp
137

We want to use a different mask for each expression type

For example

cpp
switch (Kind) {
case AVRMCExpr::VK_AVR_LO8_GS:
    Value &= 0xff;
    Value >>= 1;
    break;
  case AVRMCExpr::VK_AVR_HI8_GS:
    Value &= 0xff00;
    Value >>= 9;
    break;
  case AVRMCExpr::VK_AVR_GS:
    Value >>= 1;
    break;

Different variants require masking out different bitsets - lo8 should isolate only the lower 8-bits, hi8 should to the upper.

lib/Target/AVR/MCTargetDesc/AVRMCTargetDesc.cpp
18 ↗(On Diff #120182)

Alphabetical order

26 ↗(On Diff #120182)

Alphabetical order

lib/Target/AVR/MCTargetDesc/CMakeLists.txt
10 ↗(On Diff #120182)

Add this in alphabetical order

test/MC/AVR/relocations.s
108

Nice test

dylanmckay requested changes to this revision.Nov 24 2017, 6:52 AM
This revision now requires changes to proceed.Nov 24 2017, 6:52 AM
xiangzhai updated this revision to Diff 124321.Nov 26 2017, 7:04 PM
xiangzhai edited edge metadata.

Dear Dylan,

Thanks for your review! I updated my patch as your suggestion, please review it again.

Regards,
Leslie Zhai

xiangzhai marked 13 inline comments as done.Nov 26 2017, 7:07 PM
xiangzhai added inline comments.
lib/Target/AVR/MCTargetDesc/AVRMCELFStreamer.h
42 ↗(On Diff #120182)

I don't want to override EmitValue, sorry for my FunctionName misleading!

test/MC/AVR/relocations.s
108

Thanks :)

arsenm resigned from this revision.Nov 28 2017, 6:42 PM
dylanmckay requested changes to this revision.Dec 1 2017, 1:42 PM

@xiangzhai once the new comments are addressed, this should be good to commit

lib/Target/AVR/AsmParser/AVRAsmParser.cpp
628

Nitpick: Can we move this up to line 624 where we get the parser?

649

Same here regarding moving the variable up to 624

lib/Target/AVR/MCTargetDesc/AVRFixupKinds.h
119–120

We can probably remove this comment now that the fixups are always lowered to relocations

lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp
110

If I remember correctly, HH8 means bit 16-23, and so this mask will mask out the bits we actually want

Change the mask to 0xff0000

113

Same here, but mask should be 0xff000000

122

This needs the updated mask from the other HH8 fixup

This revision now requires changes to proceed.Dec 1 2017, 1:42 PM
xiangzhai updated this revision to Diff 125256.Dec 1 2017, 10:44 PM
xiangzhai edited edge metadata.
xiangzhai marked an inline comment as done.

Dear Dylan,

Please review it again, thanks a lot!

Regards,
Leslie Zhai

xiangzhai marked 5 inline comments as done.Dec 1 2017, 10:46 PM
dylanmckay accepted this revision.Dec 6 2017, 10:33 PM

Patch is looking great, thanks Leslie!

This revision is now accepted and ready to land.Dec 6 2017, 10:33 PM
This revision was automatically updated to reflect the committed changes.