Adding a method to TargetLowering makes sense, sure.  (I don’t think we need to modify the IR because the difference doesn’t affect IR optimizations.)

 

-Eli

 

From: llvm-dev  On Behalf Of carl-llvm-dev@petosoft.com via llvm-devSent: Tuesday, February 19, 2019 12:38 PMTo: LLVM Developers Mailing List Subject: [EXT] [llvm-dev] AVR is little endian, but requires function arguments to be in a "big endian" order, might need an additional data layout variable unless someone can suggest a better fix?

 

I think this is broken in at least one place when legalising the DAG.

This llvm ir: %3 = call { i16, i1 } @llvm.umul.with.overflow.i16(i16 %2, i16 11)

Fails to lower correctly on AVR but the problem is, unfortunately, not just coming from the AVR Target code and I am not sure it can be cleanly fixed just there. (But I would be very happy to be proved wrong as I'm very new to this.)

 The above code is taken by the legalizer and turned into a call to __mulsi3, but turns the 16 bit parameters into 32 bit parameters by adding 16 bits of 0 to the top 16 bits of each.

Unfortunately, to do this, it changes the code to be a call to __mulsi3 with four 16 bit parameters instead of two 32 bit parameters. It orders these parameters based on the endianness of the platform, read from the data layout. Unfortunately that doesn't match the AVR ABI, which expects 32 bit parameters to be passed in four consecutive registers, yet generally takes parameters in a sort of "big endian" order.

Details can be found on my bug report to the AVR team: https://github.com/avr-rust/rust/issues/129

In order to get around this, the only way to deal with this I can think of is to add an extra field to the data layout for ABIEndianness or something like that, because otherwise the information that these were 32 bit parameters, not two 16 bit parameters has been lost after legalization.

 Any help much appreciated!

 

Thanks,

Carl

 

p.s. I came up with a sample/demo PR for the sort of thing I'm talking about. Definitely not a finished product, just for discussion: https://github.com/avr-rust/llvm/pull/8.

">

(original) (raw)

OK, FYI, that fix was a good start. It solved half the problem. Registers are now correct when calling __mulsi3 from AVR code, unfortunately the return values are ordered wrongly in registers. This is probably the same sort of thing for the return handling code. I'll investigate further. PR submitted to Dylan (the AVR maintainer) in the meantime. Marked as WIP as it's not complete and also missing unit tests.

https://github.com/avr-rust/llvm/pull/9

Carl

-----Original Message-----
From: "carl-llvm-dev@petosoft.com via llvm-dev"
Sent: Wednesday, February 20, 2019 5:16am
To: "Eli Friedman"
Cc: "LLVM Development List"
Subject: Re: \[llvm-dev\] AVR is little endian, but requires function arguments to be in a "big endian" order, might need an additional data layout variable unless someone can suggest a better fix?

Cool, looking into that as an option, sounds ideal. Thanks for the steer. :)

I'll get back to you guys with results and a patch (probably via the maintainer, Dylan).

Carl

-----Original Message-----
From: "Eli Friedman"
Sent: Tuesday, February 19, 2019 3:44pm
To: "carl-llvm-dev@petosoft.com" , "LLVM Development List"
Subject: RE: \[llvm-dev\] AVR is little endian, but requires function arguments to be in a "big endian" order, might need an additional data layout variable unless someone can suggest a better fix?

Adding a method to TargetLowering makes sense, sure. (I don’t think we need to modify the IR because the difference doesn’t affect IR optimizations.)

-Eli

From: llvm-dev On Behalf Of carl-llvm-dev@petosoft.com via llvm-dev
Sent: Tuesday, February 19, 2019 12:38 PM
To: LLVM Developers Mailing List
Subject: \[EXT\] \[llvm-dev\] AVR is little endian, but requires function arguments to be in a "big endian" order, might need an additional data layout variable unless someone can suggest a better fix?

I think this is broken in at least one place when legalising the DAG.

This llvm ir:
%3 = call { i16, i1 } @llvm.umul.with.overflow.i16(i16 %2, i16 11)

Fails to lower correctly on AVR but the problem is, unfortunately, not just coming from the AVR Target code and I am not sure it can be cleanly fixed just there. (But I would be very happy to be proved wrong as I'm very new to this.)


The above code is taken by the legalizer and turned into a call to \_\_mulsi3, but turns the 16 bit parameters into 32 bit parameters by adding 16 bits of 0 to the top 16 bits of each.

Unfortunately, to do this, it changes the code to be a call to \_\_mulsi3 with four 16 bit parameters instead of two 32 bit parameters. It orders these parameters based on the endianness of the platform, read from the data layout. Unfortunately that doesn't match the AVR ABI, which expects 32 bit parameters to be passed in four consecutive registers, yet generally takes parameters in a sort of "big endian" order.

Details can be found on my bug report to the AVR team: https://github.com/avr-rust/rust/issues/129

In order to get around this, the only way to deal with this I can think of is to add an extra field to the data layout for ABIEndianness or something like that, because otherwise the information that these were 32 bit parameters, not two 16 bit parameters has been lost after legalization.


Any help much appreciated!

Thanks,

Carl

p.s. I came up with a sample/demo PR for the sort of thing I'm talking about. Definitely not a finished product, just for discussion: https://github.com/avr-rust/llvm/pull/8.