avr-llvm / llvm

[MERGED UPSTREAM] AVR backend for the LLVM compiler library
220 stars 21 forks source link

sbiw, adiw require a CPU feature not currently enabled #150

Closed seanmkauffman closed 9 years ago

seanmkauffman commented 9 years ago

I built the latest version of AVR LLVM, which has come a long way in the last couple of months, but I'm running into an issue compiling to object code with a source file I'm using.

I can generate assembly from the file using the AVR LLVM backend, but I can't reduce it to object code using the AVR LLVM tools. If I try to go directly from the IR, I get a very unhelpful LLVM ERROR: expected relocatable expression with no indication of what is causing it. If I try to use llvm-mc to assemble the .s file, I get errors about sbiw and adiw requiring a CPU feature not currently enabled.

$ ~/workspace/avr-work/bin/clang -O0 --target=avr-none-none -emit-llvm -I/usr/avr/include/ -S aes.c -o aes.ll
$ ~/workspace/avr-work/bin/llc -mtriple=avr-none -mcpu=atmega2560 -filetype=obj aes.ll -o aes.o 
LLVM ERROR: expected relocatable expression
 $ ~/workspace/avr-work/bin/llc -mtriple=avr-none -mcpu=atmega2560 -filetype=asm aes.ll -o aes.s
 $ ~/workspace/avr-work/bin/llvm-mc -triple=avr-none -mcpu=atmega2560 -filetype=obj aes.s -o aes.o
aes.s:22:2: error: instruction requires a CPU feature not currently enabled
        sbiw    r28, 24
        ^
aes.s:243:2: error: instruction requires a CPU feature not currently enabled
        adiw    r28, 24
        ^
aes.s:1507:2: error: instruction requires a CPU feature not currently enabled
        sbiw    r30, 32
        ^
aes.s:1938:2: error: instruction requires a CPU feature not currently enabled
        sbiw    r30, 32
        ^
aes.s:2088:2: error: instruction requires a CPU feature not currently enabled
        sbiw    r28, 42
        ^
aes.s:7258:2: error: instruction requires a CPU feature not currently enabled
        adiw    r28, 42
        ^
aes.s:7287:2: error: instruction requires a CPU feature not currently enabled
        sbiw    r28, 42
        ^
aes.s:12457:2: error: instruction requires a CPU feature not currently enabled
        adiw    r28, 42
        ^

(note I'm glossing over some warnings generated by clang, but those are related to the program, not to this project)

I'm opening this issue in the hopes that this problem is not enormously complicated to solve. I'd love to provide more information for debugging purposes. I'm really not sure how to isolate this into a simpler test case. The source file in question can be found here

dylanmckay commented 9 years ago

Hi Sean,

The adiw/sbiw problem is easily solved - it was a bug in the description of the features of the avr6 family of devices - it is now fixed in 52e422e482623cf73d6b3f501354bed25ab9bc60.

The expected relocatable expression is a deeper problem - if you can give me the LLVM IR file that triggers it, I will investigate :) I have encountered a bug before with the same message.

It also looks as if you are using AVR-LLVM as you had to several months ago to work around bugs. This is no longer necessary.

Could you test the patch and see if it works? I'll wait until then to close the issue.

Cheers!

seanmkauffman commented 9 years ago

Hey Dylan, I tried the updated code and now using llvm-mc to assemble the .s file also spits out LLVM ERROR: expected relocatable expression.

I actually need the IR for other reasons, so my workflow hasn't changed much. I'll email you the file, since I don't know that there's a good way to attach it to this issue.

agnat commented 9 years ago

Hi @seanmkauffman

If possible please put the IR in a gist and paste the URL. That way we have it on record and more people can take a look.

seanmkauffman commented 9 years ago

Thanks @agnat! Here's the gist

dylanmckay commented 9 years ago

We currently support the syntax:

sbci r30, lo8(-mysymbol)

ISel is generating an instruction of the form

sbci r30, lo8(-mysymbol + 1000)

Which is well defined in the context of relocations, we just aren't handling this case.

As GAS does not handle the negative lo8/hi8/etc thingos, we have no backwards compatibility problems. I am changing the syntax to -lo8(mysymbol) because in reality, it is everything that is in the brackets that get negated.

This is the only way we can support adding a constant on the end while still maintaining the proper meaning.

i.e. -lo8(mysymbol + 1000) makes sense, but lo8(-mysymbol + 1000) doesn't.

I'm working on this now.

agnat commented 9 years ago

Right... Yes, that's deep. There are so many places where we mess with add and sub and snoop around in address computations. Not easy to find the right fix. ;)

Hoisting the negation out of the modifier makes sense, however.

Ping me for review.

dylanmckay commented 9 years ago

Hoisting the negation out of the modifier makes sense, however. Ping me for review.

Will do :)

dylanmckay commented 9 years ago

Fixed in 03b893d5f4bd37acc1322775df2bd6eaf56bad63.

Ping me for review.

I forgot to do this - oh the joy of git push. Feel free to review the commit however :)

agnat commented 9 years ago

Hehe... use branches... like always ;)

Feel free to review the commit however :)

Wow. I'll give it a read. :)