Re: [PATCH 9/24] make atomic_read() behave consistently on ia64

Chris Snook
2007-08-10T19:52:11Z

Luck, Tony wrote:
>> +#define atomic_read(v) (*(volatile __s32 *)&(v)->counter)
>> +#define atomic64_read(v) (*(volatile __s64 *)&(v)->counter)
>>
>> #define atomic_set(v,i) (((v)->counter) = (i))
>> #define atomic64_set(v,i) (((v)->counter) = (i))
>
>
> Losing the volatile from the "set" variants definitely changes
> the code generated. Before the patch gcc would give us:
>
> st4.rel [r37]=r9
>
> after
> st4 [r37]=r9
>
> It is unclear whether anyone relies on (or even whether they should
> rely on) the release semantics that are provided by the current
> version of atomic.h. But making this change would require an
> audit of all the uses of atomic_set() to find an answer.
>
> There is a more worrying difference in the generated code (this
> from the ancient and venerable gcc 3.4.6 that is on my build
> machine). In rwsem_down_failed_common I see this change (after
> disassembling vmlinux, I used sed to zap the low 32-bits of addresses
> to make the diff manageable ... that's why the addresses all end
> in xxxxxxxx):
>
> 712868,712873c712913,712921
> < a0000001xxxxxxxx: adds r16=-1,r30
> < a0000001xxxxxxxx: [MII] ld8.acq r33=[r32]
> < a0000001xxxxxxxx: nop.i 0x0;;
> < a0000001xxxxxxxx: add r42=r33,r16
> < a0000001xxxxxxxx: [MMI] mov.m ar.ccv=r33;;
> < a0000001xxxxxxxx: cmpxchg8.acq r34=[r32],r42,ar.ccv
> ---
>> a0000001xxxxxxxx: adds r16=-1,r31
>> a0000001xxxxxxxx: [MMI] ld4.acq r14=[r32];;
>> a0000001xxxxxxxx: nop.m 0x0
>> a0000001xxxxxxxx: sxt4 r34=r14
>> a0000001xxxxxxxx: [MMI] nop.m 0x0;;
>> a0000001xxxxxxxx: nop.m 0x0
>> a0000001xxxxxxxx: add r15=r34,r16
>> a0000001xxxxxxxx: [MMI] mov.m ar.ccv=r34;;
>> a0000001xxxxxxxx: cmpxchg8.acq r42=[r32],r15,ar.ccv
>
> This code is probably from the rwsem_atomic_update(adjustment, sem) macro
> which is cpp'd to atomic64_add_return(). It looks really bad that the new
> code reads a 32-bit value and sign extends it rather than reading a 64-bit
> value (but I'm perplexed as to why this patch provoked this change in the
> generated code).
>
> -Tony

That's distressing. I'm about to resubmit with a volatile cast in
atomic_set as well, since people expect that behavior and I've been
shown a legitimate case where it could matter. Does the assembly look
right with that cast in atomic_set() as well?

-- Chris