Re: [bug] cbi optimization error for 8-bit AVRs

On 08/11/14 21:42, Szikra István wrote:
> On 2014.11.08. 13:48, David Brown wrote:
>> On 08/11/14 01:32, Szikra István wrote:
>>> Hi everyone!
>>>
>>> My problem in sort: I’m getting
>>>      in    r24, 0x18
>>>      ldi    r25, 0x00
>>>      andi    r24, 0xEF
>>>      out    0x18, r24
>>> instead of
>>>      cbi    0x18, 4
>>> .
>>>
>>> I’m trying to write efficient modern C/C++ code for multiple platforms
>>> including AVR 8 bit controllers.
>>>
>>> Unfortunately GCC support for AVR (among other things) is not always
>>> flawless. And it changes from versions to version (and not always for
>>> the better).
>>> Since I’m a long time AVR developer I have a lot of compiler versions
>>> installed (WinAVR 2006-2010, and Atmel Studio 6.2 with GCC 4.8.1), but I
>>> could test my code with only one (the latest).
>>>
>>> I run into some trouble with clearing port bits not translating from C
>>> into cbi in assembler. It is caused by my bits template, but I do not
>>> know why. It seems to me like a bug in GCC. Maybe someone here can shed
>>> some light on the reason, or suggest a fix.
>>>
>>> Here is the code:
>>>
>>> #include <avr/io.h>
>>> //#include <bits.hpp>
>>>
>>> template<typename T>
>>> constexpr unsigned int bits(T idx1)
>>> {
>>>      return (1<<idx1);
>>> }
>>> template <typename T, typename... Rest>
>>> constexpr unsigned int bits(T idx1, Rest... r)
>>> {
>>>      return (1<<idx1) | bits(r...);
>>> }
>>>
>>
>> Just an idea that might be worth trying - try replacing the "unsigned
>> int" return type with "uint8_t" (/always/ use <stdint.h> types with
>> defined bit sizes!).  AVR gcc has quite a number of optimisations and
>> special case patterns for dealing with 8-bit types, especially uint8_t,
>> and it often gives tighter code if you've used uint8_t even if uint16_t
>> gives the same actual values.
> 
> Thanks, I have already tried using unsigned char return type, and also
> casting it. It did not help.
>         PORTB &=~ (uint8_t)bits(4);
>   28:    88 b3           in    r24, 0x18    ; 24
>   2a:    90 e0           ldi    r25, 0x00    ; 0
>   2c:    8f 7e           andi    r24, 0xEF    ; 239
>   2e:    88 bb           out    0x18, r24    ; 24
> 
> But, what helped was casting the result of ~:
>         PORTB &= (uint8_t)~bits(4);
>   28:    c4 98           cbi    0x18, 4    ; 24
> 
> 
>>
>> Secondly, consider making your templates based on the value of idx1
>> rather than the type - the type will always be an int (or promoted to an
>> int in the shift expression).  Alternatively, drop the template entirely.
> 
> Yeah, I just replaced the bits(idx) with (1<<idx) in my code, till the
> bug is fixed. Now I'm changing it to casting...
> _port &= (unsigned char)~bits(_index);
> 
> What do you mean by basing the template on the value of idx1?
> I'm still new to templates. Actually I don't need T at all.
> 
> constexpr unsigned int bits(int idx1)
> {
>     return (1<<idx1);
> }
> template <typename... Rest>
> constexpr unsigned int bits(int idx1, Rest... r)
> {
>     return (1<<idx1) | bits(r...);
> }
> 
> 
> Regards,
> Steven

You can make templates based on constant values, with the syntax:

template<int N>
...

where N is a compile-time constant.

But in this case, it would make the source code uglier (using
"bits<4>()" instead of "bits(4)"), and would not help code generation at
all.

You only actually need templates here to get the variable number of
arguments (without using C-style varadic functions, which are horrible).
 So you can drop the templating on T:

constexpr uint8_t bits(int i) {
    return 1 << i;
}

template<typename... Rest>
constexpr uint8_t bits(int i, Rest... r) {
    return (1 << i) | bits(r...);
}


None of this affects the generated code.


At the moment, the only version of avrgcc I have convenient is avr-gcc
4.5.1 - I don't do much AVR development, and haven't updated for a
while.  But this version generates optimal object code for your
templates (after I replace "constexpr" by "const", since gcc 4.5 doesn't
support "constexpr").  So this looks like a regression to me.  I seem to
remember the same issue having come up before in the past, but I can't
remember the details.