On Wed, Dec 23, 2015 at 2:39 PM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On December 23, 2015 10:39:17 AM GMT+01:00, Uros Bizjak <ubiz...@gmail.com>
> wrote:
>>Hello!
>>
>>There is a logic error in Honza's patch "Transparent alias suport part
>>10" [1]. The part in memrefs_conflict_p should be changed to:
>>
>>- /* If decls are different or we know by offsets that there is no
>>overlap,
>>- we win. */
>>- if (!cmp || !offset_overlap_p (c, xsize, ysize))
>>+ /* If decls are different and we know by offsets that
>>+ there is no overlap, we win. */
>>+ if (!cmp && !offset_overlap_p (c, xsize, ysize))
>> return 0;
>>- /* Decls may or may not be different and offsets overlap....*/
>>+ /* Decls are different and offsets overlap....*/
>>
>>Even if decls are different, their offsets shouldn't overlap!
>
> Comparing offsets of different decls does not make sense.
Please consider two variables, say bytes, located at the neighbouring
locations, not aligned to 8 bytes. Now, first byte is to be written by
first using 8-byte aligned DImode load, change the byte and write full
DImode value back. When second byte is to be written, the same
procedure is used, but its AND aligned address aliases the location of
the first byte. So, we have:
- load the first 8-byte word from 8-byte aligned address, enclosing
the first variable
- change the byte
- store the first 8-byte word to 8-byte aligned address
- load the second 8-byte word from 8-byte aligned address (that
aliases with the address above), enclosing the second variable
- change the byte
- store the second 8-byte word to 8-byte aligned address
Now scheduler comes into play and swaps line 3 and 4:
- load the first 8-byte word from 8-byte aligned address, enclosing
the first variable
- change the byte
- load the second 8-byte word from 8-byte aligned address (that
aliases with the address above), enclosing the second variable
- store the first 8-byte word to 8-byte aligned address
- change the byte
- store the second 8-byte word to 8-byte aligned address
So, the second store killed the value of the first store.
Using the proposed logic, two variables aliases each other by xsize
that is increased according to AND operand and ysize, increased in the
same way.
>>Addresses with alignment ANDs depend on increased xsize and ysize, so
>>no wonder gcc fails to bootstrap on alpha.
>
> Alpha also fails to honor the C++ memory model and introduces store data
> races.
> I wonder if alpha needs to simply pad all decls appropriately.
alphaev68 works in the correct way, but it still uses AND addresses.
Uros.
> Maybe also finally time to ditch this arch
> (Or affected sub-arch)?
>
> Richard.
>
>>
>>In addition to this, the check for SYMBOL_REFs in base_alias_check
>>should be moved after checks for addresses.
>>
>>The patch also adds some simplification. If
>>symtab_address::equal_address_to returns -1 for "unknown" (as is
>>otherwise customary throughout the sources), we can simplify
>>compare_base_decls a bit.
>>
>>2015-12-23 Uros Bizjak <ubiz...@gmail.com>
>>
>> PR middle-end/68999
>> * symtab.c (symtab_node::equal_address_to): Return -1 instead of 2
>> if we can't determine address equivalence.
>> * alias.c (compare_base_decl): Update for changed return value of
>> symtab_node::equal_address_to.
>> (memrefs_conflict_p): Return 0 when decls are different
>> and offsets don't overlap.
>> (base_alias_check): Move check for addresses with alignment ANDs
>> before the call for compare_base_decls.
>>
>>The patch was bootstrapped and regression tested on x86_64-linux-gnu
>>{,-m32} and alpha-linux-gnu [2], which is a massive user of addresses
>>with alignment ANDs
>>
>>OK for mainline?
>>
>>[1] https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01076.html
>>[2] https://gcc.gnu.org/ml/gcc-testresults/2015-12/msg02372.html
>>
>>Uros.
>
>
