Re: volatile correctness: combine vs. target.md

[Georg-Johann Lay <avr at gjlay dot de>]

Ian Lance Taylor wrote:
> Georg-Johann Lay writes:
>
> > If general_operand can be perceived as
> >
> > (define_predicate "general_operand"
> >   (ior (match_operand 0 "memory_operand")
> >        (match_operand 0 "register_operand")
> >        (match_operand 0 "immediate_operand")))
> >
> > how can low_io_mem ever match?
>
>
> Oh, I see, I did misunderstand your question.  Sorry about that.
>
> In general combine is just going to honor what your backend is asking
> for.  If your backend says that it is OK to combine a volatile memory
> operand, then that is what combine is going to do.

Supplying an insn is the backend's way of telling that it is capable of 
performing a specific operation.

In the present case, the backend tells that it has a "Conditional Jump 
     depending on a bit in the I/O area".

It does *not* say "It is ok to infringe volatile correctness" and move 
one volatile memory access across an other one or similar memory 
annotation like memory clobber by means of inline assembly or built-in 
barrier.

> It's certainly OK in
> general to combine across a volatile memory operand, as is happening
> here.  I guess you are asking whether combine should have another check:
> if some operand in the insn is a volatile MEM, and it will cross a
> volatile MEM not mentioned in the combine, should combine reject that
> combination.

It's never correct to exchange volatile accesses.

> It's a case that will never ordinarily arise.  It only
> arises for you because you are specifically trying to combine a volatile
> MEM.  I don't know if it makes sense for the general purpose combine to
> try to handle such an unusual special case.

It's not unusual because:

* It's not unusual to write down SFRs as violatile memory like
  (*((volatile unsigned int*) 0x1234)) or as a cast to a composite
  that reflects the SFRs bit(field)s.

* It's not unusual that microcontrollers can access specific parts
  of memory -- in particular I/O -- with special instructions.

And then: combine does not operate in empty space ;-) Just like other 
general purpose parts of the compiler that have to care for exceptions 
for this hardware here and that hardware there like in reload, in 
scheduler, and many other places.

BTW, I see this bug only with 4.6. 4.7 does not combine across the 
volatile access. But it also fails to combine the jump-on-I/O-bit if the 
load from I/O immediately preceedes the conditional jump-on-bit.

Didn't yet look into that.

> Have you tried using a peephole instead of a combine pattern?
>
> Ian

I don't like RTL/text peepholes. They are just a last resort to fix 
stuff that wasn't optimized properly in other passes and to clean up 
remains from reload.

In this case, a jump-on-I/O-bit does not require a register whereas 
loading and then jump-on-bit-in-register will allocate a register even 
though it's optimized out after relaod. And if reload has fun and spills 
for some reason, peephole will never get a chance to fix it.

For setting a bit in I/O it's even worse. This can be accomplisheb by 
one instruction and is an atomic operation.

A load-bitop-write operation, however, is no more atomic and requires a 
register of a special class to perform bitop.

In order to get it atomic again, you'd have to turn IRQs off which makes 
it even more costly up to factor of 6 because it is slower, more code, 
needs a scratch to store the interrupt-flag and increases IRQ respond times.

What about that predicate?

(define_predicate "low_io_mem"
   (and (match_code "mem")
        (match_test "low_io_address_operand (XEXP (op, 0))")))

Guess it operates the same because the drag-volatile-over-volatile 
happens not in recog(_for_combine) but already when combine synthesizes 
new patterns.  As combine juggles with data flow, it must not exchange 
volatiles. That's all. The backend will know if it's ok to place a 
specific operand into a specific insn or not.

And skipping optimizations like these altogether because they are 
uncomfortable to handle is the wrong direction, IMHO.

Johann