Re: [avr-gcc-list] Confusing volatile behaviour with 4.6.2

[Georg-Johann Lay]

Wim Lewis schrieb:
> On 11/28/11 5:33 AM, Andy Warner wrote:
>
> > What is it about this construct that makes the compiler feel free to reorder ?
> > I assume this must be a very specific use case I've tripped over, if
> > volatiles were being reordered wholesale in 4.6.2, I'd expect there to
> > be many, many problems surfacing.
>
> Me too. Perhaps whatever optimizer rule produces the 'sbis' instruction
> doesn't understand that it's reordering volatile operations? Turning on
> RTL dumps suggests that the problem shows up after the combine pass.
> Maybe someone more familiar with gcc internals could comment.
>
> Running the delta minimizer on the code you posted reduces the test case
> to this:
>
> typedef unsigned char uint8_t;
> #define SEARCHING (-2)
> #define UCSR0A (*(volatile uint8_t *)((0x0B) + 0x20))
> #define UDR0 (*(volatile uint8_t *)((0x0C) + 0x20))
> void __vector_18(void)
> {
>         unsigned char status = UCSR0A;
>         unsigned char data = UDR0;
>         static volatile int slot;
>         if (status & 0x10) {
>                 if (slot == SEARCHING) {
>                         slot ++;
>                 }
>         }
> }

Thanks for supplying a non-obfuscated test case, this makes things much 
easier.

> avr-gcc-4.6.2 -S -mmcu=atmega128 -Wall -fverbose-asm -O2 produces:
> [....]
> __vector_18:
> /* prologue: function */
> /* frame size = 0 */
> /* stack size = 0 */
> .L__stack_usage = 0
>         in r24,44-0x20   ;  data, MEM[(volatile uint8_t *)44B]
>         sbis 43-0x20,4   ; ,
>         rjmp .L1         ;
> [...]

Well, in avr.md we have

;; Lower half of the I/O space - use sbic/sbis directly.
(define_insn "*sbix_branch"
  [(set (pc)
        (if_then_else
         (match_operator 0 "eqne_operator"
                         [(zero_extract:HI
                           (mem:QI (match_operand 1 "low_io_address_operand" 
"n"))
                           (const_int 1)
                           (match_operand 2 "const_int_operand" "n"))
                          (const_int 0)])
         (label_ref (match_operand 3 "" ""))
         (pc)))]
  "(optimize > 0)"
  "* return avr_out_sbxx_branch (insn, operands);"

The problem is that there is

   (mem:QI (match_operand 1 "low_io_address_operand"))

instead of

   (match_operand:QI 1 "low_io_memory_operand"))

Actually, there is no low_io_memory_operand predicate, but if there was 
one it would solve this problem.

The reason is that a plain mem is not sensitive to volatile (resp. 
volatile_ok) but a memory predicate is.

It was written that way because otherwise, the pattern would *never* 
match because the only reason to have it is to produce SBIS/SBIC which 
only work on I/O which is volatile.

I am really unsure of combine can be blamed here because the operand 
that it moves over the other volatile access (UDR0) is just an integer, 
namely a low_io_address_operand one.

I'd suggest to ask in address@hidden if insn combine may do it.

If yes, we are really in trouble because there is no obvious way how 
avr.md can fix it without kicking out SBIS/SBIC/SBI/SBC.

Johann