Re: [PATCH] Fix PR 91708
On Fri, 13 Sep 2019, Bernd Edlinger wrote:
> On 9/13/19 1:23 PM, Richard Biener wrote:
> > On Thu, 12 Sep 2019, Bernd Edlinger wrote:
> >
> >> On 9/12/19 10:08 AM, Richard Biener wrote:
> >>> On Wed, 11 Sep 2019, Bernd Edlinger wrote:
> >>>
> On 9/11/19 8:30 PM, Richard Biener wrote:
> >>>
> >>> More like the following? I wonder if we can assert that
> >>> MEM_NOTRAP_P () are equal (see all the for_gcse checks in exp_equiv_p).
> >>> But as said earlier I wonder in which cases a replacement is
> >>> profitable at all - thus, if a
> >>>
> >>> else if (MEM_P (trial))
> >>> /* Do not replace anything with a MEM. */
> >>> ;
> >>>
> >>
> >> Yes, I like that better, since there is essentially nothing stopping
> >> it from replacing a REG with a MEM, except the rtxcost function, maybe.
> >>
> >> It turned out that the previous version got into an endless loop here,
> >> since the loop termination happens when replacing MEM by itself, except
> >> when something else terminates the search.
> >
> > Is that how it works without the patch as well? I admit the loop
> > is a bit hard to follow since it is not only iterating
> > over elt->next_same_value ...
> >
>
> Yes, that seems to be the case, since the memory reference itself
> is always in the set. There was either an endless loop, when
> the src variable was set this if can always be taken, which makes
> no progress:
>
> else if (src
>&& preferable (src_cost, src_regcost,
> src_eqv_cost, src_eqv_regcost) <= 0
>&& preferable (src_cost, src_regcost,
> src_related_cost, src_related_regcost) <= 0
>&& preferable (src_cost, src_regcost,
> src_elt_cost, src_elt_regcost) <= 0)
> trial = src, src_cost = MAX_COST;
>
> or there was a crash when the above was not taken then:
>
> else
> {
> trial = elt->exp;
> elt = elt->next_same_value;
> src_elt_cost = MAX_COST;
> }
>
> crashes, because elt == NULL at some point.
>
> >> So how about this?
> >>
> >> The only possible MEM->MEM transformations are now:
> >> - replacing trial = dest (result mov dest, dest; which is a no-op insn)
> >> - replacing trial = src (which is a no-op transformation, and exits the
> >> loop)
> >>
> >> Therefore the overlapping mem move handling no longer necessary.
> >>
> >>
> >> Bootstrap and reg-tested on x86_64-pc-linux-gnu.
> >> Is it OK for trunk?
> >
> > Looks OK to me. Can you see if this has any unexpected code-generation
> > effects? I would expect it to not make a difference at all, but you
> > never know -- any differences in cc1files?
> >
>
> I tried to install the patch after _stage1_ was competed,
> but there was no bootstrap miscompare (on x86_64-pc-linux-gnu)
> so, no this does not make any difference here.
The patch is OK for trunk then.
Thanks,
Richard.
Re: [PATCH] Fix PR 91708
On 9/13/19 1:23 PM, Richard Biener wrote:
> On Thu, 12 Sep 2019, Bernd Edlinger wrote:
>
>> On 9/12/19 10:08 AM, Richard Biener wrote:
>>> On Wed, 11 Sep 2019, Bernd Edlinger wrote:
>>>
On 9/11/19 8:30 PM, Richard Biener wrote:
>>>
>>> More like the following? I wonder if we can assert that
>>> MEM_NOTRAP_P () are equal (see all the for_gcse checks in exp_equiv_p).
>>> But as said earlier I wonder in which cases a replacement is
>>> profitable at all - thus, if a
>>>
>>> else if (MEM_P (trial))
>>> /* Do not replace anything with a MEM. */
>>> ;
>>>
>>
>> Yes, I like that better, since there is essentially nothing stopping
>> it from replacing a REG with a MEM, except the rtxcost function, maybe.
>>
>> It turned out that the previous version got into an endless loop here,
>> since the loop termination happens when replacing MEM by itself, except
>> when something else terminates the search.
>
> Is that how it works without the patch as well? I admit the loop
> is a bit hard to follow since it is not only iterating
> over elt->next_same_value ...
>
Yes, that seems to be the case, since the memory reference itself
is always in the set. There was either an endless loop, when
the src variable was set this if can always be taken, which makes
no progress:
else if (src
&& preferable (src_cost, src_regcost,
src_eqv_cost, src_eqv_regcost) <= 0
&& preferable (src_cost, src_regcost,
src_related_cost, src_related_regcost) <= 0
&& preferable (src_cost, src_regcost,
src_elt_cost, src_elt_regcost) <= 0)
trial = src, src_cost = MAX_COST;
or there was a crash when the above was not taken then:
else
{
trial = elt->exp;
elt = elt->next_same_value;
src_elt_cost = MAX_COST;
}
crashes, because elt == NULL at some point.
>> So how about this?
>>
>> The only possible MEM->MEM transformations are now:
>> - replacing trial = dest (result mov dest, dest; which is a no-op insn)
>> - replacing trial = src (which is a no-op transformation, and exits the loop)
>>
>> Therefore the overlapping mem move handling no longer necessary.
>>
>>
>> Bootstrap and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>
> Looks OK to me. Can you see if this has any unexpected code-generation
> effects? I would expect it to not make a difference at all, but you
> never know -- any differences in cc1files?
>
I tried to install the patch after _stage1_ was competed,
but there was no bootstrap miscompare (on x86_64-pc-linux-gnu)
so, no this does not make any difference here.
Bernd.
> Not really my primary area of expertise...
>
> Thanks,
> Richard.
>
Re: [PATCH] Fix PR 91708
On Thu, 12 Sep 2019, Bernd Edlinger wrote: > On 9/12/19 10:08 AM, Richard Biener wrote: > > On Wed, 11 Sep 2019, Bernd Edlinger wrote: > > > >> On 9/11/19 8:30 PM, Richard Biener wrote: > > > > More like the following? I wonder if we can assert that > > MEM_NOTRAP_P () are equal (see all the for_gcse checks in exp_equiv_p). > > But as said earlier I wonder in which cases a replacement is > > profitable at all - thus, if a > > > > else if (MEM_P (trial)) > > /* Do not replace anything with a MEM. */ > > ; > > > > Yes, I like that better, since there is essentially nothing stopping > it from replacing a REG with a MEM, except the rtxcost function, maybe. > > It turned out that the previous version got into an endless loop here, > since the loop termination happens when replacing MEM by itself, except > when something else terminates the search. Is that how it works without the patch as well? I admit the loop is a bit hard to follow since it is not only iterating over elt->next_same_value ... > So how about this? > > The only possible MEM->MEM transformations are now: > - replacing trial = dest (result mov dest, dest; which is a no-op insn) > - replacing trial = src (which is a no-op transformation, and exits the loop) > > Therefore the overlapping mem move handling no longer necessary. > > > Bootstrap and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? Looks OK to me. Can you see if this has any unexpected code-generation effects? I would expect it to not make a difference at all, but you never know -- any differences in cc1files? Not really my primary area of expertise... Thanks, Richard.
Re: [PATCH] Fix PR 91708
On 9/12/19 10:08 AM, Richard Biener wrote:
> On Wed, 11 Sep 2019, Bernd Edlinger wrote:
>
>> On 9/11/19 8:30 PM, Richard Biener wrote:
>
> More like the following? I wonder if we can assert that
> MEM_NOTRAP_P () are equal (see all the for_gcse checks in exp_equiv_p).
> But as said earlier I wonder in which cases a replacement is
> profitable at all - thus, if a
>
> else if (MEM_P (trial))
> /* Do not replace anything with a MEM. */
> ;
>
Yes, I like that better, since there is essentially nothing stopping
it from replacing a REG with a MEM, except the rtxcost function, maybe.
It turned out that the previous version got into an endless loop here,
since the loop termination happens when replacing MEM by itself, except
when something else terminates the search.
So how about this?
The only possible MEM->MEM transformations are now:
- replacing trial = dest (result mov dest, dest; which is a no-op insn)
- replacing trial = src (which is a no-op transformation, and exits the loop)
Therefore the overlapping mem move handling no longer necessary.
Bootstrap and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?
Thanks
Bernd.
2019-09-12 Bernd Edlinger
PR middle-end/91708
* cse.c (cse_insn): Do not replace anything with a
MEM.
--- gcc/cse.c.orig 2019-07-24 21:21:53.590065924 +0200
+++ gcc/cse.c 2019-09-12 12:50:18.566801155 +0200
@@ -5238,23 +5238,6 @@ cse_insn (rtx_insn *insn)
src_elt_cost = MAX_COST;
}
- /* Avoid creation of overlapping memory moves. */
- if (MEM_P (trial) && MEM_P (dest) && !rtx_equal_p (trial, dest))
- {
- rtx src, dest;
-
- /* BLKmode moves are not handled by cse anyway. */
- if (GET_MODE (trial) == BLKmode)
- break;
-
- src = canon_rtx (trial);
- dest = canon_rtx (SET_DEST (sets[i].rtl));
-
- if (!MEM_P (src) || !MEM_P (dest)
- || !nonoverlapping_memrefs_p (src, dest, false))
- break;
- }
-
/* Try to optimize
(set (reg:M N) (const_int A))
(set (reg:M2 O) (const_int B))
@@ -5385,6 +5368,12 @@ cse_insn (rtx_insn *insn)
/* Do nothing for this case. */
;
+ /* Do not replace anything with a MEM, except the replacement
+ is a no-op. This allows this loop to terminate. */
+ else if (MEM_P (trial) && !rtx_equal_p (trial, SET_SRC(sets[i].rtl)))
+ /* Do nothing for this case. */
+ ;
+
/* Look for a substitution that makes a valid insn. */
else if (validate_unshare_change (insn, _SRC (sets[i].rtl),
trial, 0))
Re: [PATCH] Fix PR 91708
On Wed, 11 Sep 2019, Bernd Edlinger wrote:
> On 9/11/19 8:30 PM, Richard Biener wrote:
> > On September 11, 2019 7:41:22 PM GMT+02:00, Bernd Edlinger
> > wrote:
> >> On 9/11/19 6:08 PM, Jeff Law wrote:
> >>> On 9/11/19 7:49 AM, Bernd Edlinger wrote:
> On 9/11/19 9:23 AM, Richard Biener wrote:
> > On Tue, 10 Sep 2019, Bernd Edlinger wrote:
> >
> >> Hi!
> >>
> >> This ICE happens when compiling real_nextafter in real.c.
> >> CSE sees this:
> >>
> >> (insn 179 178 180 11 (set (reg:SI 319)
> >> (reg/v/f:SI 273 [ rD.73757 ]))
> >> "../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
> >> (nil))
> >> [...]
> >> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
> >> [(voidD.73 *)r_77(D)]+0 S4 A8])
> >> (unspec:SI [
> >> (reg:SI 320)
> >> ] UNSPEC_UNALIGNED_STORE))
> >> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> >> (nil))
> >> [...]
> >> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [
> >> rD.73757 ])
> >> (const_int 20 [0x14])) [0 MEM
> >> [(voidD.73 *)r_77(D)]+20 S4 A8])
> >> (unspec:SI [
> >> (reg:SI 320)
> >> ] UNSPEC_UNALIGNED_STORE))
> >> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> >> (expr_list:REG_DEAD (reg:SI 320)
> >> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
> >> (nil
> >> [...]
> >> (insn 234 233 235 11 (set (reg:SI 340)
> >> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM >> int> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
> >> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
> >> (nil))
> >>
> >>
> >> ... and transforms insn 234 in an invalid insn:
> >>
> >>
> >> (insn 234 233 235 11 (set (reg:SI 340 [ MEM
> >> [(struct real_valueD.28367 *)r_77(D)] ])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> >> (const_int 20 [0x14])) [0 MEM
> >> [(voidD.73 *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9
> >> 643 {*thumb2_movsi_vfp}
> >> (nil))
> >>
> >> which triggers the assertion in the arm back-end, because the MEM
> >> is not aligned.
> >>
> >> To fix that I changed exp_equiv_p to consider MEMs with different
> >> MEM_ALIGN or
> >> ALIAS_SET as different.
> >
> > I think the appropriate fix is to retain the mem_attrs of the
> >> original
> > MEM since obviously the replacement does not change those? It's a
> >> mere
> > change of the MEMs address but implementation-wise CSE replaces the
> >> whole
> > MEM?
> >
> > For CSE exp_equiv_p simply compares the addresses, only if for_gcse
> > we do further checks like mem_attrs_eq_p. IMHO that is correct.
> >
> > I'm not sure what CSE does above, that is, what does it think it
> >> does?
> > It doesn't replace the loaded value, it seems to do sth else which
> > is odd for CSE (replaces a REG with a PLUS - but why?)
> >
>
> What I think CSE is thinking there is this:
>
> insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0
> >> MEM [(voidD.73 *)r_77(D)]+0 S4 A8]
> that is the same address as where insn 234 reads the value back but
> >> there we know it is aligned.
>
> insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [
> >> rD.73757 ]) (const_int 20 [0x14])) [0 MEM [(voidD.73
> >> *)r_77(D)]+20 S4 A8]
>
> But since both reg:SI 320 is dead and reg:SI 319 is dead as well,
> >> when
> insn 234 processed, it replaces one mem with another mem that has
> >> the same
> value.
> >>> Just to make sure I understand. Are you saying the addresses for the
> >>> MEMs are equal or the contents of the memory location are equal.
> >>>
> >>
> >> The MEMs all have different addresses, but the same value, they are
> >>from a
> >> memset or something:
> >>
> >> (gdb) call dump_class(elt)
> >> Equivalence chain for (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM
> >> [(void *)r_77(D)]+0 S4 A8]):
> >> (unspec:SI [
> >>(reg:SI 320)
> >>] UNSPEC_UNALIGNED_STORE)
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> >> (const_int 20 [0x14])) [0 MEM [(void *)r_77(D)]+20 S4 A8])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> >> (const_int 16 [0x10])) [0 MEM [(void *)r_77(D)]+16 S4 A8])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> >> (const_int 12 [0xc])) [0 MEM [(void *)r_77(D)]+12 S4 A8])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> >> (const_int 8 [0x8])) [0 MEM [(void *)r_77(D)]+8 S4 A8])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> >> (const_int 4 [0x4])) [0 MEM [(void *)r_77(D)]+4 S4 A8])
> >> (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void *)r_77(D)]+0
> >> S4 A8])
> >>
> >>
> >> The insn 234, uses the same address as the last in the list
> >> (mem:SI (reg/v/f:SI 273 [
Re: [PATCH] Fix PR 91708
On 9/11/19 8:30 PM, Richard Biener wrote:
> On September 11, 2019 7:41:22 PM GMT+02:00, Bernd Edlinger
> wrote:
>> On 9/11/19 6:08 PM, Jeff Law wrote:
>>> On 9/11/19 7:49 AM, Bernd Edlinger wrote:
On 9/11/19 9:23 AM, Richard Biener wrote:
> On Tue, 10 Sep 2019, Bernd Edlinger wrote:
>
>> Hi!
>>
>> This ICE happens when compiling real_nextafter in real.c.
>> CSE sees this:
>>
>> (insn 179 178 180 11 (set (reg:SI 319)
>> (reg/v/f:SI 273 [ rD.73757 ]))
>> "../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
>> (nil))
>> [...]
>> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
>> [(voidD.73 *)r_77(D)]+0 S4 A8])
>> (unspec:SI [
>> (reg:SI 320)
>> ] UNSPEC_UNALIGNED_STORE))
>> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
>> (nil))
>> [...]
>> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [
>> rD.73757 ])
>> (const_int 20 [0x14])) [0 MEM
>> [(voidD.73 *)r_77(D)]+20 S4 A8])
>> (unspec:SI [
>> (reg:SI 320)
>> ] UNSPEC_UNALIGNED_STORE))
>> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
>> (expr_list:REG_DEAD (reg:SI 320)
>> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
>> (nil
>> [...]
>> (insn 234 233 235 11 (set (reg:SI 340)
>> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM > int> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
>> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
>> (nil))
>>
>>
>> ... and transforms insn 234 in an invalid insn:
>>
>>
>> (insn 234 233 235 11 (set (reg:SI 340 [ MEM
>> [(struct real_valueD.28367 *)r_77(D)] ])
>> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
>> (const_int 20 [0x14])) [0 MEM
>> [(voidD.73 *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9
>> 643 {*thumb2_movsi_vfp}
>> (nil))
>>
>> which triggers the assertion in the arm back-end, because the MEM
>> is not aligned.
>>
>> To fix that I changed exp_equiv_p to consider MEMs with different
>> MEM_ALIGN or
>> ALIAS_SET as different.
>
> I think the appropriate fix is to retain the mem_attrs of the
>> original
> MEM since obviously the replacement does not change those? It's a
>> mere
> change of the MEMs address but implementation-wise CSE replaces the
>> whole
> MEM?
>
> For CSE exp_equiv_p simply compares the addresses, only if for_gcse
> we do further checks like mem_attrs_eq_p. IMHO that is correct.
>
> I'm not sure what CSE does above, that is, what does it think it
>> does?
> It doesn't replace the loaded value, it seems to do sth else which
> is odd for CSE (replaces a REG with a PLUS - but why?)
>
What I think CSE is thinking there is this:
insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0
>> MEM [(voidD.73 *)r_77(D)]+0 S4 A8]
that is the same address as where insn 234 reads the value back but
>> there we know it is aligned.
insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [
>> rD.73757 ]) (const_int 20 [0x14])) [0 MEM [(voidD.73
>> *)r_77(D)]+20 S4 A8]
But since both reg:SI 320 is dead and reg:SI 319 is dead as well,
>> when
insn 234 processed, it replaces one mem with another mem that has
>> the same
value.
>>> Just to make sure I understand. Are you saying the addresses for the
>>> MEMs are equal or the contents of the memory location are equal.
>>>
>>
>> The MEMs all have different addresses, but the same value, they are
>>from a
>> memset or something:
>>
>> (gdb) call dump_class(elt)
>> Equivalence chain for (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM
>> [(void *)r_77(D)]+0 S4 A8]):
>> (unspec:SI [
>>(reg:SI 320)
>>] UNSPEC_UNALIGNED_STORE)
>> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>> (const_int 20 [0x14])) [0 MEM [(void *)r_77(D)]+20 S4 A8])
>> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>> (const_int 16 [0x10])) [0 MEM [(void *)r_77(D)]+16 S4 A8])
>> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>> (const_int 12 [0xc])) [0 MEM [(void *)r_77(D)]+12 S4 A8])
>> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>> (const_int 8 [0x8])) [0 MEM [(void *)r_77(D)]+8 S4 A8])
>> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>> (const_int 4 [0x4])) [0 MEM [(void *)r_77(D)]+4 S4 A8])
>> (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void *)r_77(D)]+0
>> S4 A8])
>>
>>
>> The insn 234, uses the same address as the last in the list
>> (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void *)r_77(D)]+0
>> S4 A8])
>> but incompatible alignment and alias set.
>>
>> since we only are interested in the value CSE picks the most silly
>> variant,
>> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>> (const_int 20 [0x14])) [0 MEM [(void *)r_77(D)]+20 S4 A8])
>>
>> now this
Re: [PATCH] Fix PR 91708
On September 11, 2019 7:41:22 PM GMT+02:00, Bernd Edlinger
wrote:
>On 9/11/19 6:08 PM, Jeff Law wrote:
>> On 9/11/19 7:49 AM, Bernd Edlinger wrote:
>>> On 9/11/19 9:23 AM, Richard Biener wrote:
On Tue, 10 Sep 2019, Bernd Edlinger wrote:
> Hi!
>
> This ICE happens when compiling real_nextafter in real.c.
> CSE sees this:
>
> (insn 179 178 180 11 (set (reg:SI 319)
> (reg/v/f:SI 273 [ rD.73757 ]))
>"../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
> (nil))
> [...]
> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
> [(voidD.73 *)r_77(D)]+0 S4 A8])
> (unspec:SI [
> (reg:SI 320)
> ] UNSPEC_UNALIGNED_STORE))
>"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> (nil))
> [...]
> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [
>rD.73757 ])
> (const_int 20 [0x14])) [0 MEM
>[(voidD.73 *)r_77(D)]+20 S4 A8])
> (unspec:SI [
> (reg:SI 320)
> ] UNSPEC_UNALIGNED_STORE))
>"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> (expr_list:REG_DEAD (reg:SI 320)
> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
> (nil
> [...]
> (insn 234 233 235 11 (set (reg:SI 340)
> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM int> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
>"../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
> (nil))
>
>
> ... and transforms insn 234 in an invalid insn:
>
>
> (insn 234 233 235 11 (set (reg:SI 340 [ MEM
>[(struct real_valueD.28367 *)r_77(D)] ])
> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> (const_int 20 [0x14])) [0 MEM
>[(voidD.73 *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9
>643 {*thumb2_movsi_vfp}
> (nil))
>
> which triggers the assertion in the arm back-end, because the MEM
>is not aligned.
>
> To fix that I changed exp_equiv_p to consider MEMs with different
>MEM_ALIGN or
> ALIAS_SET as different.
I think the appropriate fix is to retain the mem_attrs of the
>original
MEM since obviously the replacement does not change those? It's a
>mere
change of the MEMs address but implementation-wise CSE replaces the
>whole
MEM?
For CSE exp_equiv_p simply compares the addresses, only if for_gcse
we do further checks like mem_attrs_eq_p. IMHO that is correct.
I'm not sure what CSE does above, that is, what does it think it
>does?
It doesn't replace the loaded value, it seems to do sth else which
is odd for CSE (replaces a REG with a PLUS - but why?)
>>>
>>> What I think CSE is thinking there is this:
>>>
>>> insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0
>MEM [(voidD.73 *)r_77(D)]+0 S4 A8]
>>> that is the same address as where insn 234 reads the value back but
>there we know it is aligned.
>>>
>>> insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [
>rD.73757 ]) (const_int 20 [0x14])) [0 MEM [(voidD.73
>*)r_77(D)]+20 S4 A8]
>>>
>>> But since both reg:SI 320 is dead and reg:SI 319 is dead as well,
>when
>>> insn 234 processed, it replaces one mem with another mem that has
>the same
>>> value.
>> Just to make sure I understand. Are you saying the addresses for the
>> MEMs are equal or the contents of the memory location are equal.
>>
>
>The MEMs all have different addresses, but the same value, they are
>from a
>memset or something:
>
>(gdb) call dump_class(elt)
>Equivalence chain for (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM
> [(void *)r_77(D)]+0 S4 A8]):
>(unspec:SI [
>(reg:SI 320)
>] UNSPEC_UNALIGNED_STORE)
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>(const_int 20 [0x14])) [0 MEM [(void *)r_77(D)]+20 S4 A8])
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>(const_int 16 [0x10])) [0 MEM [(void *)r_77(D)]+16 S4 A8])
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> (const_int 12 [0xc])) [0 MEM [(void *)r_77(D)]+12 S4 A8])
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> (const_int 8 [0x8])) [0 MEM [(void *)r_77(D)]+8 S4 A8])
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> (const_int 4 [0x4])) [0 MEM [(void *)r_77(D)]+4 S4 A8])
>(mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void *)r_77(D)]+0
>S4 A8])
>
>
>The insn 234, uses the same address as the last in the list
>(mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void *)r_77(D)]+0
>S4 A8])
>but incompatible alignment and alias set.
>
>since we only are interested in the value CSE picks the most silly
>variant,
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>(const_int 20 [0x14])) [0 MEM [(void *)r_77(D)]+20 S4 A8])
>
>now this has a wrong alias set, a wrong alignment and a different
>address,
>and is much more expensive, no wonder that lra needs to rewrite that.
But the alias and alignment are properties of the expression representing the
value
Re: [PATCH] Fix PR 91708
Hi Jeff, > We're talking about two instructions where if the first executes, then > the second also executes. If the memory addresses are the same, then > their alignment is the same. > > In your case the two instructions are on different execution paths and > are in fact mutually exclusive. Sure but it shows we cannot just force the alignment to be the same in all cases. The loads are reading the same address in the same block but they are *not* identical. The unaligned stores are emitted by an inlined memset, and it *aliases* with a later load that uses the same address. Given they are really different accesses with very different meanings you can't just force the same MEM for both! The alignment of the memset could be improved (I think we still don't pass the correct alignment info), but the alias set would need to continue to be different otherwise we violate the C aliasing rules. Wilco
Re: [PATCH] Fix PR 91708
On 9/11/19 6:08 PM, Jeff Law wrote:
> On 9/11/19 7:49 AM, Bernd Edlinger wrote:
>> On 9/11/19 9:23 AM, Richard Biener wrote:
>>> On Tue, 10 Sep 2019, Bernd Edlinger wrote:
>>>
Hi!
This ICE happens when compiling real_nextafter in real.c.
CSE sees this:
(insn 179 178 180 11 (set (reg:SI 319)
(reg/v/f:SI 273 [ rD.73757 ]))
"../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
(nil))
[...]
(insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
[(voidD.73 *)r_77(D)]+0 S4 A8])
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE))
"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
(nil))
[...]
(insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
(const_int 20 [0x14])) [0 MEM [(voidD.73
*)r_77(D)]+20 S4 A8])
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE))
"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
(expr_list:REG_DEAD (reg:SI 320)
(expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
(nil
[...]
(insn 234 233 235 11 (set (reg:SI 340)
(mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM
[(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
"../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
(nil))
... and transforms insn 234 in an invalid insn:
(insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
real_valueD.28367 *)r_77(D)] ])
(mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
(const_int 20 [0x14])) [0 MEM [(voidD.73
*)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
{*thumb2_movsi_vfp}
(nil))
which triggers the assertion in the arm back-end, because the MEM is not
aligned.
To fix that I changed exp_equiv_p to consider MEMs with different
MEM_ALIGN or
ALIAS_SET as different.
>>>
>>> I think the appropriate fix is to retain the mem_attrs of the original
>>> MEM since obviously the replacement does not change those? It's a mere
>>> change of the MEMs address but implementation-wise CSE replaces the whole
>>> MEM?
>>>
>>> For CSE exp_equiv_p simply compares the addresses, only if for_gcse
>>> we do further checks like mem_attrs_eq_p. IMHO that is correct.
>>>
>>> I'm not sure what CSE does above, that is, what does it think it does?
>>> It doesn't replace the loaded value, it seems to do sth else which
>>> is odd for CSE (replaces a REG with a PLUS - but why?)
>>>
>>
>> What I think CSE is thinking there is this:
>>
>> insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0 MEM
>> [(voidD.73 *)r_77(D)]+0 S4 A8]
>> that is the same address as where insn 234 reads the value back but there we
>> know it is aligned.
>>
>> insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [
>> rD.73757 ]) (const_int 20 [0x14])) [0 MEM [(voidD.73
>> *)r_77(D)]+20 S4 A8]
>>
>> But since both reg:SI 320 is dead and reg:SI 319 is dead as well, when
>> insn 234 processed, it replaces one mem with another mem that has the same
>> value.
> Just to make sure I understand. Are you saying the addresses for the
> MEMs are equal or the contents of the memory location are equal.
>
The MEMs all have different addresses, but the same value, they are from a
memset or something:
(gdb) call dump_class(elt)
Equivalence chain for (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void
*)r_77(D)]+0 S4 A8]):
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE)
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 20 [0x14])) [0 MEM [(void *)r_77(D)]+20 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 16 [0x10])) [0 MEM [(void *)r_77(D)]+16 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 12 [0xc])) [0 MEM [(void *)r_77(D)]+12 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 8 [0x8])) [0 MEM [(void *)r_77(D)]+8 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 4 [0x4])) [0 MEM [(void *)r_77(D)]+4 S4 A8])
(mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void *)r_77(D)]+0 S4 A8])
The insn 234, uses the same address as the last in the list
(mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void *)r_77(D)]+0 S4 A8])
but incompatible alignment and alias set.
since we only are interested in the value CSE picks the most silly variant,
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 20 [0x14])) [0 MEM [(void *)r_77(D)]+20 S4 A8])
now this has a wrong alias set, a wrong alignment and a different address,
and is much more expensive, no wonder that lra needs to rewrite that.
> For the former the alignment has to be the same, plain and simple, even
> if GCC isn't aware the alignments have to be the same.
>
Yes.
> For the latter we'd
Re: [PATCH] Fix PR 91708
On 9/11/19 10:38 AM, Wilco Dijkstra wrote:
> Hi Jeff,
>
> Jeff wrote:
>> Just to make sure I understand. Are you saying the addresses for the
>> MEMs are equal or the contents of the memory location are equal.
>>
>> For the former the alignment has to be the same, plain and simple, even
>> if GCC isn't aware the alignments have to be the same.
>>
>> For the latter we'd be better off loading the data into a REG, then
>> using the REG for the source of both memory stores.
>
> The addresses are the same (they should probably have been canonicalized
> earlier, that might be a separate bug). I'm not sure why the unaligned stores
> have a lower alignment/no alias set, but it's certainly possible that a memset
> expansion of a subfield of a structure has a lower alignment.
>
> The known alignment of otherwise identical loads in different blocks could be
> different, ie:
>
> __attribute__((aligned(1)) struct { int x; } *p;
> if (((intptr_t)p & 3) == 0)
>x = p->x; // align 4
> else
>y = p->x; // align 1
>
> It would be very wrong to change the alignment of the 2nd load to be 4-byte
> aligned.
True, but that's not what's going on here.
We're talking about two instructions where if the first executes, then
the second also executes. If the memory addresses are the same, then
their alignment is the same.
In your case the two instructions are on different execution paths and
are in fact mutually exclusive.
jeff
Re: [PATCH] Fix PR 91708
Hi Jeff,
Jeff wrote:
> Just to make sure I understand. Are you saying the addresses for the
> MEMs are equal or the contents of the memory location are equal.
>
> For the former the alignment has to be the same, plain and simple, even
> if GCC isn't aware the alignments have to be the same.
>
> For the latter we'd be better off loading the data into a REG, then
> using the REG for the source of both memory stores.
The addresses are the same (they should probably have been canonicalized
earlier, that might be a separate bug). I'm not sure why the unaligned stores
have a lower alignment/no alias set, but it's certainly possible that a memset
expansion of a subfield of a structure has a lower alignment.
The known alignment of otherwise identical loads in different blocks could be
different, ie:
__attribute__((aligned(1)) struct { int x; } *p;
if (((intptr_t)p & 3) == 0)
x = p->x; // align 4
else
y = p->x; // align 1
It would be very wrong to change the alignment of the 2nd load to be 4-byte
aligned.
Wilco
Re: [PATCH] Fix PR 91708
On 9/11/19 7:49 AM, Bernd Edlinger wrote:
> On 9/11/19 9:23 AM, Richard Biener wrote:
>> On Tue, 10 Sep 2019, Bernd Edlinger wrote:
>>
>>> Hi!
>>>
>>> This ICE happens when compiling real_nextafter in real.c.
>>> CSE sees this:
>>>
>>> (insn 179 178 180 11 (set (reg:SI 319)
>>> (reg/v/f:SI 273 [ rD.73757 ]))
>>> "../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
>>> (nil))
>>> [...]
>>> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
>>> [(voidD.73 *)r_77(D)]+0 S4 A8])
>>> (unspec:SI [
>>> (reg:SI 320)
>>> ] UNSPEC_UNALIGNED_STORE))
>>> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
>>> (nil))
>>> [...]
>>> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
>>> (const_int 20 [0x14])) [0 MEM [(voidD.73
>>> *)r_77(D)]+20 S4 A8])
>>> (unspec:SI [
>>> (reg:SI 320)
>>> ] UNSPEC_UNALIGNED_STORE))
>>> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
>>> (expr_list:REG_DEAD (reg:SI 320)
>>> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
>>> (nil
>>> [...]
>>> (insn 234 233 235 11 (set (reg:SI 340)
>>> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM
>>> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
>>> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
>>> (nil))
>>>
>>>
>>> ... and transforms insn 234 in an invalid insn:
>>>
>>>
>>> (insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
>>> real_valueD.28367 *)r_77(D)] ])
>>> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
>>> (const_int 20 [0x14])) [0 MEM [(voidD.73
>>> *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
>>> {*thumb2_movsi_vfp}
>>> (nil))
>>>
>>> which triggers the assertion in the arm back-end, because the MEM is not
>>> aligned.
>>>
>>> To fix that I changed exp_equiv_p to consider MEMs with different MEM_ALIGN
>>> or
>>> ALIAS_SET as different.
>>
>> I think the appropriate fix is to retain the mem_attrs of the original
>> MEM since obviously the replacement does not change those? It's a mere
>> change of the MEMs address but implementation-wise CSE replaces the whole
>> MEM?
>>
>> For CSE exp_equiv_p simply compares the addresses, only if for_gcse
>> we do further checks like mem_attrs_eq_p. IMHO that is correct.
>>
>> I'm not sure what CSE does above, that is, what does it think it does?
>> It doesn't replace the loaded value, it seems to do sth else which
>> is odd for CSE (replaces a REG with a PLUS - but why?)
>>
>
> What I think CSE is thinking there is this:
>
> insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0 MEM
> [(voidD.73 *)r_77(D)]+0 S4 A8]
> that is the same address as where insn 234 reads the value back but there we
> know it is aligned.
>
> insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757
> ]) (const_int 20 [0x14])) [0 MEM [(voidD.73 *)r_77(D)]+20 S4
> A8]
>
> But since both reg:SI 320 is dead and reg:SI 319 is dead as well, when
> insn 234 processed, it replaces one mem with another mem that has the same
> value.
Just to make sure I understand. Are you saying the addresses for the
MEMs are equal or the contents of the memory location are equal.
For the former the alignment has to be the same, plain and simple, even
if GCC isn't aware the alignments have to be the same.
For the latter we'd be better off loading the data into a REG, then
using the REG for the source of both memory stores.
Jeff
Re: [PATCH] Fix PR 91708
On 9/11/19 7:37 AM, Richard Biener wrote:
> On Wed, 11 Sep 2019, Bernd Edlinger wrote:
>
>> On 9/11/19 12:43 AM, Jeff Law wrote:
>>> On 9/10/19 1:51 PM, Bernd Edlinger wrote:
Hi!
This ICE happens when compiling real_nextafter in real.c.
CSE sees this:
(insn 179 178 180 11 (set (reg:SI 319)
(reg/v/f:SI 273 [ rD.73757 ]))
"../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
(nil))
[...]
(insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
[(voidD.73 *)r_77(D)]+0 S4 A8])
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE))
"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
(nil))
[...]
(insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
(const_int 20 [0x14])) [0 MEM [(voidD.73
*)r_77(D)]+20 S4 A8])
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE))
"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
(expr_list:REG_DEAD (reg:SI 320)
(expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
(nil
[...]
(insn 234 233 235 11 (set (reg:SI 340)
(mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM
[(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
"../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
(nil))
... and transforms insn 234 in an invalid insn:
(insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
real_valueD.28367 *)r_77(D)] ])
(mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
(const_int 20 [0x14])) [0 MEM [(voidD.73
*)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
{*thumb2_movsi_vfp}
(nil))
which triggers the assertion in the arm back-end, because the MEM is not
aligned.
To fix that I changed exp_equiv_p to consider MEMs with different
MEM_ALIGN or
ALIAS_SET as different.
This patch fixes the arm bootstrap for --with-cpu=cortex-a57
--with-mode=thumb --with-fpu=fp-armv8 --with-float=hard
which I confirmed using a cross compiler. And it fixes the test case that
is attached to the PR, but it is way
too large for the test suite.
Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?
Thanks
Bernd.
patch-pr91708.diff
2019-09-10 Bernd Edlinger
PR middle-end/91708
* cse.c (exp_equiv_p): Consider MEMs with different
alias set or alignment as different.
>>> Hmm, I would have expected the address space and alignment checks to be
>>> handled by the call to mem_attrs_eq_p.
>>>
>>
>> Yes, but exp_equiv_p is called from here:
>>
>> lookup (rtx x, unsigned int hash, machine_mode mode)
>> {
>> struct table_elt *p;
>>
>> for (p = table[hash]; p; p = p->next_same_hash)
>> if (mode == p->mode && ((x == p->exp && REG_P (x))
>> || exp_equiv_p (x, p->exp, !REG_P (x), false)))
>> return p;
>>
>> with for_gcse = false si mem_attrs_eq_p is not used.
>>
>>> Which aspect of the MEMs is really causing the problem here?
>>>
>>
>> The alignment is (formally) different, but since also the address is
>> different,
>> the alignment could be also be really wrong.
>>
>> Originally it was [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]
>> but it is replaced with [0 MEM [(voidD.73 *)r_77(D)]+20 S4
>> A8],
>> that appears to be done, because there is the same value that was in the
>> original
>> location.
>>
>> But also the wrong alias set will cause problems if the instruction
>> is re-materialized in another place (which actually happened in LRA and
>> caused the
>> assertion, BTW).
>
> But !for_gcse shouldn't do this kind of things with MEMs. It should
> only replace a MEM with a REG, not put in some other "equivalent"
> MEMs.
Agreed, generally replacing one MEM with another MEM would be silly.
Though I wouldn't be terribly surprised if CSE did so if the form of one
of the MEMs was cheaper than the other.
It be better to use a REG though.
jeff
Re: [PATCH] Fix PR 91708
On September 11, 2019 4:41:10 PM GMT+02:00, Bernd Edlinger
wrote:
>On 9/11/19 3:55 PM, Richard Biener wrote:
>> On Wed, 11 Sep 2019, Bernd Edlinger wrote:
>>
>>> On 9/11/19 9:23 AM, Richard Biener wrote:
On Tue, 10 Sep 2019, Bernd Edlinger wrote:
> Hi!
>
> This ICE happens when compiling real_nextafter in real.c.
> CSE sees this:
>
> (insn 179 178 180 11 (set (reg:SI 319)
> (reg/v/f:SI 273 [ rD.73757 ]))
>"../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
> (nil))
> [...]
> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
> [(voidD.73 *)r_77(D)]+0 S4 A8])
> (unspec:SI [
> (reg:SI 320)
> ] UNSPEC_UNALIGNED_STORE))
>"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> (nil))
> [...]
> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [
>rD.73757 ])
> (const_int 20 [0x14])) [0 MEM
>[(voidD.73 *)r_77(D)]+20 S4 A8])
> (unspec:SI [
> (reg:SI 320)
> ] UNSPEC_UNALIGNED_STORE))
>"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> (expr_list:REG_DEAD (reg:SI 320)
> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
> (nil
> [...]
> (insn 234 233 235 11 (set (reg:SI 340)
> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM int> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
>"../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
> (nil))
>
>
> ... and transforms insn 234 in an invalid insn:
>
>
> (insn 234 233 235 11 (set (reg:SI 340 [ MEM
>[(struct real_valueD.28367 *)r_77(D)] ])
> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> (const_int 20 [0x14])) [0 MEM
>[(voidD.73 *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9
>643 {*thumb2_movsi_vfp}
> (nil))
>
> which triggers the assertion in the arm back-end, because the MEM
>is not aligned.
>
> To fix that I changed exp_equiv_p to consider MEMs with different
>MEM_ALIGN or
> ALIAS_SET as different.
I think the appropriate fix is to retain the mem_attrs of the
>original
MEM since obviously the replacement does not change those? It's a
>mere
change of the MEMs address but implementation-wise CSE replaces the
>whole
MEM?
For CSE exp_equiv_p simply compares the addresses, only if for_gcse
we do further checks like mem_attrs_eq_p. IMHO that is correct.
I'm not sure what CSE does above, that is, what does it think it
>does?
It doesn't replace the loaded value, it seems to do sth else which
is odd for CSE (replaces a REG with a PLUS - but why?)
>>>
>>> What I think CSE is thinking there is this:
>>>
>>> insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0
>MEM [(voidD.73 *)r_77(D)]+0 S4 A8]
>>> that is the same address as where insn 234 reads the value back but
>there we know it is aligned.
>>>
>>> insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [
>rD.73757 ]) (const_int 20 [0x14])) [0 MEM [(voidD.73
>*)r_77(D)]+20 S4 A8]
>>>
>>> But since both reg:SI 320 is dead and reg:SI 319 is dead as well,
>when
>>> insn 234 processed, it replaces one mem with another mem that has
>the same
>>> value.
>>
>> But why? I see zero benefit doing that. Note the addresses are
>equal,
>> so if it for some reason is beneficial CSE could still simply
>> _preserve_ the original mem_attrs.
>>
>
>Yes, but the addresses are simply *not* equal.
>
>--- cse.c.orig 2019-07-24 21:21:53.590065924 +0200
>+++ cse.c 2019-09-11 16:03:43.429236836 +0200
>@@ -4564,6 +4564,10 @@
> else if (GET_CODE (x) == PARALLEL)
> sets = XALLOCAVEC (struct set, XVECLEN (x, 0));
>
>+ if (INSN_UID(insn) == 234 && GET_CODE(x) == SET
>+ && GET_CODE(SET_DEST(x)) == REG && REGNO(SET_DEST(x)) == 340
>+ && getenv("debug")) asm("int3");
>+
> this_insn = insn;
> /* Records what this insn does to set CC0. */
> this_insn_cc0 = 0;
>
>(gdb) n
>4809 elt = lookup (src, sets[i].src_hash, mode);
>(gdb) call debug(src)
>(mem:SI (reg/v/f:SI 273 [ r ]) [52 MEM [(struct
>real_value *)r_77(D)]+0 S4 A32])
>(gdb) call dump_class(elt)
>Equivalence chain for (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM
> [(void *)r_77(D)]+0 S4 A8]):
>(unspec:SI [
>(reg:SI 320)
>] UNSPEC_UNALIGNED_STORE)
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>(const_int 20 [0x14])) [0 MEM [(void *)r_77(D)]+20 S4 A8])
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
>(const_int 16 [0x10])) [0 MEM [(void *)r_77(D)]+16 S4 A8])
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> (const_int 12 [0xc])) [0 MEM [(void *)r_77(D)]+12 S4 A8])
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> (const_int 8 [0x8])) [0 MEM [(void *)r_77(D)]+8 S4 A8])
>(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> (const_int 4 [0x4])) [0 MEM [(void *)r_77(D)]+4 S4 A8])
Re: [PATCH] Fix PR 91708
On 9/11/19 3:55 PM, Richard Biener wrote:
> On Wed, 11 Sep 2019, Bernd Edlinger wrote:
>
>> On 9/11/19 9:23 AM, Richard Biener wrote:
>>> On Tue, 10 Sep 2019, Bernd Edlinger wrote:
>>>
Hi!
This ICE happens when compiling real_nextafter in real.c.
CSE sees this:
(insn 179 178 180 11 (set (reg:SI 319)
(reg/v/f:SI 273 [ rD.73757 ]))
"../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
(nil))
[...]
(insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
[(voidD.73 *)r_77(D)]+0 S4 A8])
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE))
"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
(nil))
[...]
(insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
(const_int 20 [0x14])) [0 MEM [(voidD.73
*)r_77(D)]+20 S4 A8])
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE))
"../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
(expr_list:REG_DEAD (reg:SI 320)
(expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
(nil
[...]
(insn 234 233 235 11 (set (reg:SI 340)
(mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM
[(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
"../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
(nil))
... and transforms insn 234 in an invalid insn:
(insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
real_valueD.28367 *)r_77(D)] ])
(mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
(const_int 20 [0x14])) [0 MEM [(voidD.73
*)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
{*thumb2_movsi_vfp}
(nil))
which triggers the assertion in the arm back-end, because the MEM is not
aligned.
To fix that I changed exp_equiv_p to consider MEMs with different
MEM_ALIGN or
ALIAS_SET as different.
>>>
>>> I think the appropriate fix is to retain the mem_attrs of the original
>>> MEM since obviously the replacement does not change those? It's a mere
>>> change of the MEMs address but implementation-wise CSE replaces the whole
>>> MEM?
>>>
>>> For CSE exp_equiv_p simply compares the addresses, only if for_gcse
>>> we do further checks like mem_attrs_eq_p. IMHO that is correct.
>>>
>>> I'm not sure what CSE does above, that is, what does it think it does?
>>> It doesn't replace the loaded value, it seems to do sth else which
>>> is odd for CSE (replaces a REG with a PLUS - but why?)
>>>
>>
>> What I think CSE is thinking there is this:
>>
>> insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0 MEM
>> [(voidD.73 *)r_77(D)]+0 S4 A8]
>> that is the same address as where insn 234 reads the value back but there we
>> know it is aligned.
>>
>> insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [
>> rD.73757 ]) (const_int 20 [0x14])) [0 MEM [(voidD.73
>> *)r_77(D)]+20 S4 A8]
>>
>> But since both reg:SI 320 is dead and reg:SI 319 is dead as well, when
>> insn 234 processed, it replaces one mem with another mem that has the same
>> value.
>
> But why? I see zero benefit doing that. Note the addresses are equal,
> so if it for some reason is beneficial CSE could still simply
> _preserve_ the original mem_attrs.
>
Yes, but the addresses are simply *not* equal.
--- cse.c.orig 2019-07-24 21:21:53.590065924 +0200
+++ cse.c 2019-09-11 16:03:43.429236836 +0200
@@ -4564,6 +4564,10 @@
else if (GET_CODE (x) == PARALLEL)
sets = XALLOCAVEC (struct set, XVECLEN (x, 0));
+ if (INSN_UID(insn) == 234 && GET_CODE(x) == SET
+ && GET_CODE(SET_DEST(x)) == REG && REGNO(SET_DEST(x)) == 340
+ && getenv("debug")) asm("int3");
+
this_insn = insn;
/* Records what this insn does to set CC0. */
this_insn_cc0 = 0;
(gdb) n
4809elt = lookup (src, sets[i].src_hash, mode);
(gdb) call debug(src)
(mem:SI (reg/v/f:SI 273 [ r ]) [52 MEM [(struct real_value
*)r_77(D)]+0 S4 A32])
(gdb) call dump_class(elt)
Equivalence chain for (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void
*)r_77(D)]+0 S4 A8]):
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE)
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 20 [0x14])) [0 MEM [(void *)r_77(D)]+20 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 16 [0x10])) [0 MEM [(void *)r_77(D)]+16 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 12 [0xc])) [0 MEM [(void *)r_77(D)]+12 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 8 [0x8])) [0 MEM [(void *)r_77(D)]+8 S4 A8])
(mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
(const_int 4 [0x4])) [0 MEM [(void *)r_77(D)]+4 S4 A8])
(mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM [(void *)r_77(D)]+0 S4 A8])
[...]
5393
Re: [PATCH] Fix PR 91708
On Wed, 11 Sep 2019, Bernd Edlinger wrote:
> On 9/11/19 9:23 AM, Richard Biener wrote:
> > On Tue, 10 Sep 2019, Bernd Edlinger wrote:
> >
> >> Hi!
> >>
> >> This ICE happens when compiling real_nextafter in real.c.
> >> CSE sees this:
> >>
> >> (insn 179 178 180 11 (set (reg:SI 319)
> >> (reg/v/f:SI 273 [ rD.73757 ]))
> >> "../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
> >> (nil))
> >> [...]
> >> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
> >> [(voidD.73 *)r_77(D)]+0 S4 A8])
> >> (unspec:SI [
> >> (reg:SI 320)
> >> ] UNSPEC_UNALIGNED_STORE))
> >> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> >> (nil))
> >> [...]
> >> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> >> (const_int 20 [0x14])) [0 MEM [(voidD.73
> >> *)r_77(D)]+20 S4 A8])
> >> (unspec:SI [
> >> (reg:SI 320)
> >> ] UNSPEC_UNALIGNED_STORE))
> >> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> >> (expr_list:REG_DEAD (reg:SI 320)
> >> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
> >> (nil
> >> [...]
> >> (insn 234 233 235 11 (set (reg:SI 340)
> >> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM
> >> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
> >> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
> >> (nil))
> >>
> >>
> >> ... and transforms insn 234 in an invalid insn:
> >>
> >>
> >> (insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
> >> real_valueD.28367 *)r_77(D)] ])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> >> (const_int 20 [0x14])) [0 MEM [(voidD.73
> >> *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
> >> {*thumb2_movsi_vfp}
> >> (nil))
> >>
> >> which triggers the assertion in the arm back-end, because the MEM is not
> >> aligned.
> >>
> >> To fix that I changed exp_equiv_p to consider MEMs with different
> >> MEM_ALIGN or
> >> ALIAS_SET as different.
> >
> > I think the appropriate fix is to retain the mem_attrs of the original
> > MEM since obviously the replacement does not change those? It's a mere
> > change of the MEMs address but implementation-wise CSE replaces the whole
> > MEM?
> >
> > For CSE exp_equiv_p simply compares the addresses, only if for_gcse
> > we do further checks like mem_attrs_eq_p. IMHO that is correct.
> >
> > I'm not sure what CSE does above, that is, what does it think it does?
> > It doesn't replace the loaded value, it seems to do sth else which
> > is odd for CSE (replaces a REG with a PLUS - but why?)
> >
>
> What I think CSE is thinking there is this:
>
> insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0 MEM
> [(voidD.73 *)r_77(D)]+0 S4 A8]
> that is the same address as where insn 234 reads the value back but there we
> know it is aligned.
>
> insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757
> ]) (const_int 20 [0x14])) [0 MEM [(voidD.73 *)r_77(D)]+20 S4
> A8]
>
> But since both reg:SI 320 is dead and reg:SI 319 is dead as well, when
> insn 234 processed, it replaces one mem with another mem that has the same
> value.
But why? I see zero benefit doing that. Note the addresses are equal,
so if it for some reason is beneficial CSE could still simply
_preserve_ the original mem_attrs.
Richard.
Re: [PATCH] Fix PR 91708
On 9/11/19 9:23 AM, Richard Biener wrote:
> On Tue, 10 Sep 2019, Bernd Edlinger wrote:
>
>> Hi!
>>
>> This ICE happens when compiling real_nextafter in real.c.
>> CSE sees this:
>>
>> (insn 179 178 180 11 (set (reg:SI 319)
>> (reg/v/f:SI 273 [ rD.73757 ])) "../../gcc-trunk-1/gcc/real.c":120:10
>> 643 {*thumb2_movsi_vfp}
>> (nil))
>> [...]
>> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
>> [(voidD.73 *)r_77(D)]+0 S4 A8])
>> (unspec:SI [
>> (reg:SI 320)
>> ] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
>> 129 {unaligned_storesi}
>> (nil))
>> [...]
>> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
>> (const_int 20 [0x14])) [0 MEM [(voidD.73
>> *)r_77(D)]+20 S4 A8])
>> (unspec:SI [
>> (reg:SI 320)
>> ] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
>> 129 {unaligned_storesi}
>> (expr_list:REG_DEAD (reg:SI 320)
>> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
>> (nil
>> [...]
>> (insn 234 233 235 11 (set (reg:SI 340)
>> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM
>> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
>> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
>> (nil))
>>
>>
>> ... and transforms insn 234 in an invalid insn:
>>
>>
>> (insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
>> real_valueD.28367 *)r_77(D)] ])
>> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
>> (const_int 20 [0x14])) [0 MEM [(voidD.73
>> *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
>> {*thumb2_movsi_vfp}
>> (nil))
>>
>> which triggers the assertion in the arm back-end, because the MEM is not
>> aligned.
>>
>> To fix that I changed exp_equiv_p to consider MEMs with different MEM_ALIGN
>> or
>> ALIAS_SET as different.
>
> I think the appropriate fix is to retain the mem_attrs of the original
> MEM since obviously the replacement does not change those? It's a mere
> change of the MEMs address but implementation-wise CSE replaces the whole
> MEM?
>
> For CSE exp_equiv_p simply compares the addresses, only if for_gcse
> we do further checks like mem_attrs_eq_p. IMHO that is correct.
>
> I'm not sure what CSE does above, that is, what does it think it does?
> It doesn't replace the loaded value, it seems to do sth else which
> is odd for CSE (replaces a REG with a PLUS - but why?)
>
What I think CSE is thinking there is this:
insn 181 stores the value of (reg:SI 320) at (mem:SI (reg:SI 319) [0 MEM
[(voidD.73 *)r_77(D)]+0 S4 A8]
that is the same address as where insn 234 reads the value back but there we
know it is aligned.
insn 186 stores the same value at (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757
]) (const_int 20 [0x14])) [0 MEM [(voidD.73 *)r_77(D)]+20 S4 A8]
But since both reg:SI 320 is dead and reg:SI 319 is dead as well, when
insn 234 processed, it replaces one mem with another mem that has the same
value.
Usually that would be okay, but not when the RTX is extracted from an unspec
unalinged_storedi.
Bernd.
>> This patch fixes the arm bootstrap for --with-cpu=cortex-a57
>> --with-mode=thumb --with-fpu=fp-armv8 --with-float=hard
>> which I confirmed using a cross compiler. And it fixes the test case that
>> is attached to the PR, but it is way
>> too large for the test suite.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>
> So no, I don't think this is correct.
>
> Thanks,
> Richard.
>
Re: [PATCH] Fix PR 91708
On Wed, 11 Sep 2019, Bernd Edlinger wrote:
> On 9/11/19 12:43 AM, Jeff Law wrote:
> > On 9/10/19 1:51 PM, Bernd Edlinger wrote:
> >> Hi!
> >>
> >> This ICE happens when compiling real_nextafter in real.c.
> >> CSE sees this:
> >>
> >> (insn 179 178 180 11 (set (reg:SI 319)
> >> (reg/v/f:SI 273 [ rD.73757 ]))
> >> "../../gcc-trunk-1/gcc/real.c":120:10 643 {*thumb2_movsi_vfp}
> >> (nil))
> >> [...]
> >> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
> >> [(voidD.73 *)r_77(D)]+0 S4 A8])
> >> (unspec:SI [
> >> (reg:SI 320)
> >> ] UNSPEC_UNALIGNED_STORE))
> >> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> >> (nil))
> >> [...]
> >> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> >> (const_int 20 [0x14])) [0 MEM [(voidD.73
> >> *)r_77(D)]+20 S4 A8])
> >> (unspec:SI [
> >> (reg:SI 320)
> >> ] UNSPEC_UNALIGNED_STORE))
> >> "../../gcc-trunk-1/gcc/real.c":120:10 129 {unaligned_storesi}
> >> (expr_list:REG_DEAD (reg:SI 320)
> >> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
> >> (nil
> >> [...]
> >> (insn 234 233 235 11 (set (reg:SI 340)
> >> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM
> >> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
> >> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
> >> (nil))
> >>
> >>
> >> ... and transforms insn 234 in an invalid insn:
> >>
> >>
> >> (insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
> >> real_valueD.28367 *)r_77(D)] ])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> >> (const_int 20 [0x14])) [0 MEM [(voidD.73
> >> *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
> >> {*thumb2_movsi_vfp}
> >> (nil))
> >>
> >> which triggers the assertion in the arm back-end, because the MEM is not
> >> aligned.
> >>
> >> To fix that I changed exp_equiv_p to consider MEMs with different
> >> MEM_ALIGN or
> >> ALIAS_SET as different.
> >>
> >> This patch fixes the arm bootstrap for --with-cpu=cortex-a57
> >> --with-mode=thumb --with-fpu=fp-armv8 --with-float=hard
> >> which I confirmed using a cross compiler. And it fixes the test case that
> >> is attached to the PR, but it is way
> >> too large for the test suite.
> >>
> >>
> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> >> Is it OK for trunk?
> >>
> >>
> >> Thanks
> >> Bernd.
> >>
> >>
> >> patch-pr91708.diff
> >>
> >> 2019-09-10 Bernd Edlinger
> >>
> >>PR middle-end/91708
> >>* cse.c (exp_equiv_p): Consider MEMs with different
> >>alias set or alignment as different.
> > Hmm, I would have expected the address space and alignment checks to be
> > handled by the call to mem_attrs_eq_p.
> >
>
> Yes, but exp_equiv_p is called from here:
>
> lookup (rtx x, unsigned int hash, machine_mode mode)
> {
> struct table_elt *p;
>
> for (p = table[hash]; p; p = p->next_same_hash)
> if (mode == p->mode && ((x == p->exp && REG_P (x))
> || exp_equiv_p (x, p->exp, !REG_P (x), false)))
> return p;
>
> with for_gcse = false si mem_attrs_eq_p is not used.
>
> > Which aspect of the MEMs is really causing the problem here?
> >
>
> The alignment is (formally) different, but since also the address is
> different,
> the alignment could be also be really wrong.
>
> Originally it was [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]
> but it is replaced with [0 MEM [(voidD.73 *)r_77(D)]+20 S4
> A8],
> that appears to be done, because there is the same value that was in the
> original
> location.
>
> But also the wrong alias set will cause problems if the instruction
> is re-materialized in another place (which actually happened in LRA and
> caused the
> assertion, BTW).
But !for_gcse shouldn't do this kind of things with MEMs. It should
only replace a MEM with a REG, not put in some other "equivalent"
MEMs.
Richard.
Re: [PATCH] Fix PR 91708
On 9/11/19 12:43 AM, Jeff Law wrote:
> On 9/10/19 1:51 PM, Bernd Edlinger wrote:
>> Hi!
>>
>> This ICE happens when compiling real_nextafter in real.c.
>> CSE sees this:
>>
>> (insn 179 178 180 11 (set (reg:SI 319)
>> (reg/v/f:SI 273 [ rD.73757 ])) "../../gcc-trunk-1/gcc/real.c":120:10
>> 643 {*thumb2_movsi_vfp}
>> (nil))
>> [...]
>> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
>> [(voidD.73 *)r_77(D)]+0 S4 A8])
>> (unspec:SI [
>> (reg:SI 320)
>> ] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
>> 129 {unaligned_storesi}
>> (nil))
>> [...]
>> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
>> (const_int 20 [0x14])) [0 MEM [(voidD.73
>> *)r_77(D)]+20 S4 A8])
>> (unspec:SI [
>> (reg:SI 320)
>> ] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
>> 129 {unaligned_storesi}
>> (expr_list:REG_DEAD (reg:SI 320)
>> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
>> (nil
>> [...]
>> (insn 234 233 235 11 (set (reg:SI 340)
>> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM
>> [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]))
>> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
>> (nil))
>>
>>
>> ... and transforms insn 234 in an invalid insn:
>>
>>
>> (insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
>> real_valueD.28367 *)r_77(D)] ])
>> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
>> (const_int 20 [0x14])) [0 MEM [(voidD.73
>> *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
>> {*thumb2_movsi_vfp}
>> (nil))
>>
>> which triggers the assertion in the arm back-end, because the MEM is not
>> aligned.
>>
>> To fix that I changed exp_equiv_p to consider MEMs with different MEM_ALIGN
>> or
>> ALIAS_SET as different.
>>
>> This patch fixes the arm bootstrap for --with-cpu=cortex-a57
>> --with-mode=thumb --with-fpu=fp-armv8 --with-float=hard
>> which I confirmed using a cross compiler. And it fixes the test case that
>> is attached to the PR, but it is way
>> too large for the test suite.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> patch-pr91708.diff
>>
>> 2019-09-10 Bernd Edlinger
>>
>> PR middle-end/91708
>> * cse.c (exp_equiv_p): Consider MEMs with different
>> alias set or alignment as different.
> Hmm, I would have expected the address space and alignment checks to be
> handled by the call to mem_attrs_eq_p.
>
Yes, but exp_equiv_p is called from here:
lookup (rtx x, unsigned int hash, machine_mode mode)
{
struct table_elt *p;
for (p = table[hash]; p; p = p->next_same_hash)
if (mode == p->mode && ((x == p->exp && REG_P (x))
|| exp_equiv_p (x, p->exp, !REG_P (x), false)))
return p;
with for_gcse = false si mem_attrs_eq_p is not used.
> Which aspect of the MEMs is really causing the problem here?
>
The alignment is (formally) different, but since also the address is different,
the alignment could be also be really wrong.
Originally it was [(struct real_valueD.28367 *)r_77(D)]+0 S4 A32]
but it is replaced with [0 MEM [(voidD.73 *)r_77(D)]+20 S4 A8],
that appears to be done, because there is the same value that was in the
original
location.
But also the wrong alias set will cause problems if the instruction
is re-materialized in another place (which actually happened in LRA and caused
the
assertion, BTW).
Bernd.
Re: [PATCH] Fix PR 91708
On Tue, 10 Sep 2019, Bernd Edlinger wrote:
> Hi!
>
> This ICE happens when compiling real_nextafter in real.c.
> CSE sees this:
>
> (insn 179 178 180 11 (set (reg:SI 319)
> (reg/v/f:SI 273 [ rD.73757 ])) "../../gcc-trunk-1/gcc/real.c":120:10
> 643 {*thumb2_movsi_vfp}
> (nil))
> [...]
> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
> [(voidD.73 *)r_77(D)]+0 S4 A8])
> (unspec:SI [
> (reg:SI 320)
> ] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
> 129 {unaligned_storesi}
> (nil))
> [...]
> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> (const_int 20 [0x14])) [0 MEM [(voidD.73
> *)r_77(D)]+20 S4 A8])
> (unspec:SI [
> (reg:SI 320)
> ] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
> 129 {unaligned_storesi}
> (expr_list:REG_DEAD (reg:SI 320)
> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
> (nil
> [...]
> (insn 234 233 235 11 (set (reg:SI 340)
> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM [(struct
> real_valueD.28367 *)r_77(D)]+0 S4 A32]))
> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
> (nil))
>
>
> ... and transforms insn 234 in an invalid insn:
>
>
> (insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
> real_valueD.28367 *)r_77(D)] ])
> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> (const_int 20 [0x14])) [0 MEM [(voidD.73
> *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
> {*thumb2_movsi_vfp}
> (nil))
>
> which triggers the assertion in the arm back-end, because the MEM is not
> aligned.
>
> To fix that I changed exp_equiv_p to consider MEMs with different MEM_ALIGN or
> ALIAS_SET as different.
I think the appropriate fix is to retain the mem_attrs of the original
MEM since obviously the replacement does not change those? It's a mere
change of the MEMs address but implementation-wise CSE replaces the whole
MEM?
For CSE exp_equiv_p simply compares the addresses, only if for_gcse
we do further checks like mem_attrs_eq_p. IMHO that is correct.
I'm not sure what CSE does above, that is, what does it think it does?
It doesn't replace the loaded value, it seems to do sth else which
is odd for CSE (replaces a REG with a PLUS - but why?)
> This patch fixes the arm bootstrap for --with-cpu=cortex-a57
> --with-mode=thumb --with-fpu=fp-armv8 --with-float=hard
> which I confirmed using a cross compiler. And it fixes the test case that is
> attached to the PR, but it is way
> too large for the test suite.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
So no, I don't think this is correct.
Thanks,
Richard.
Re: [PATCH] Fix PR 91708
On 9/10/19 1:51 PM, Bernd Edlinger wrote:
> Hi!
>
> This ICE happens when compiling real_nextafter in real.c.
> CSE sees this:
>
> (insn 179 178 180 11 (set (reg:SI 319)
> (reg/v/f:SI 273 [ rD.73757 ])) "../../gcc-trunk-1/gcc/real.c":120:10
> 643 {*thumb2_movsi_vfp}
> (nil))
> [...]
> (insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
> [(voidD.73 *)r_77(D)]+0 S4 A8])
> (unspec:SI [
> (reg:SI 320)
> ] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
> 129 {unaligned_storesi}
> (nil))
> [...]
> (insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> (const_int 20 [0x14])) [0 MEM [(voidD.73
> *)r_77(D)]+20 S4 A8])
> (unspec:SI [
> (reg:SI 320)
> ] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
> 129 {unaligned_storesi}
> (expr_list:REG_DEAD (reg:SI 320)
> (expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
> (nil
> [...]
> (insn 234 233 235 11 (set (reg:SI 340)
> (mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM [(struct
> real_valueD.28367 *)r_77(D)]+0 S4 A32]))
> "../../gcc-trunk-1/gcc/real.c":5185:9 643 {*thumb2_movsi_vfp}
> (nil))
>
>
> ... and transforms insn 234 in an invalid insn:
>
>
> (insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
> real_valueD.28367 *)r_77(D)] ])
> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> (const_int 20 [0x14])) [0 MEM [(voidD.73
> *)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
> {*thumb2_movsi_vfp}
> (nil))
>
> which triggers the assertion in the arm back-end, because the MEM is not
> aligned.
>
> To fix that I changed exp_equiv_p to consider MEMs with different MEM_ALIGN or
> ALIAS_SET as different.
>
> This patch fixes the arm bootstrap for --with-cpu=cortex-a57
> --with-mode=thumb --with-fpu=fp-armv8 --with-float=hard
> which I confirmed using a cross compiler. And it fixes the test case that is
> attached to the PR, but it is way
> too large for the test suite.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.
>
>
> patch-pr91708.diff
>
> 2019-09-10 Bernd Edlinger
>
> PR middle-end/91708
> * cse.c (exp_equiv_p): Consider MEMs with different
> alias set or alignment as different.
Hmm, I would have expected the address space and alignment checks to be
handled by the call to mem_attrs_eq_p.
Which aspect of the MEMs is really causing the problem here?
jeff
[PATCH] Fix PR 91708
Hi!
This ICE happens when compiling real_nextafter in real.c.
CSE sees this:
(insn 179 178 180 11 (set (reg:SI 319)
(reg/v/f:SI 273 [ rD.73757 ])) "../../gcc-trunk-1/gcc/real.c":120:10
643 {*thumb2_movsi_vfp}
(nil))
[...]
(insn 181 180 182 11 (set (mem:SI (reg:SI 319) [0 MEM
[(voidD.73 *)r_77(D)]+0 S4 A8])
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
129 {unaligned_storesi}
(nil))
[...]
(insn 186 185 187 11 (set (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
(const_int 20 [0x14])) [0 MEM [(voidD.73
*)r_77(D)]+20 S4 A8])
(unspec:SI [
(reg:SI 320)
] UNSPEC_UNALIGNED_STORE)) "../../gcc-trunk-1/gcc/real.c":120:10
129 {unaligned_storesi}
(expr_list:REG_DEAD (reg:SI 320)
(expr_list:REG_DEAD (reg/f:SI 319 [ rD.73757 ])
(nil
[...]
(insn 234 233 235 11 (set (reg:SI 340)
(mem:SI (reg/v/f:SI 273 [ rD.73757 ]) [52 MEM [(struct
real_valueD.28367 *)r_77(D)]+0 S4 A32])) "../../gcc-trunk-1/gcc/real.c":5185:9
643 {*thumb2_movsi_vfp}
(nil))
... and transforms insn 234 in an invalid insn:
(insn 234 233 235 11 (set (reg:SI 340 [ MEM [(struct
real_valueD.28367 *)r_77(D)] ])
(mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
(const_int 20 [0x14])) [0 MEM [(voidD.73
*)r_77(D)]+20 S4 A8])) "../../gcc-trunk-1/gcc/real.c":5185:9 643
{*thumb2_movsi_vfp}
(nil))
which triggers the assertion in the arm back-end, because the MEM is not
aligned.
To fix that I changed exp_equiv_p to consider MEMs with different MEM_ALIGN or
ALIAS_SET as different.
This patch fixes the arm bootstrap for --with-cpu=cortex-a57 --with-mode=thumb
--with-fpu=fp-armv8 --with-float=hard
which I confirmed using a cross compiler. And it fixes the test case that is
attached to the PR, but it is way
too large for the test suite.
Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?
Thanks
Bernd.
2019-09-10 Bernd Edlinger
PR middle-end/91708
* cse.c (exp_equiv_p): Consider MEMs with different
alias set or alignment as different.
--- gcc/cse.c.orig 2019-07-24 21:21:53.590065924 +0200
+++ gcc/cse.c 2019-09-10 16:15:37.899933738 +0200
@@ -2637,8 +2637,13 @@ exp_equiv_p (const_rtx x, const_rtx y, i
if (GET_MODE (x) != GET_MODE (y))
return 0;
- /* MEMs referring to different address space are not equivalent. */
- if (code == MEM && MEM_ADDR_SPACE (x) != MEM_ADDR_SPACE (y))
+ /* MEMs referring to different address spaces are not equivalent.
+ MEMs with different alias sets are not equivalent either.
+ Also the MEM_ALIGN needs to be identical in order not to break
+ constraints of insn's that need certain alignment (see PR91708). */
+ if (code == MEM && (MEM_ADDR_SPACE (x) != MEM_ADDR_SPACE (y)
+ || MEM_ALIAS_SET (x) != MEM_ALIAS_SET (y)
+ || MEM_ALIGN (x) != MEM_ALIGN (y)))
return 0;
switch (code)
