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
> > <bernd.edlin...@hotmail.de> 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
> >> <charD.7[1:24]> [(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 <charD.7[1:24]>
> >> [(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 <unsigned
> >> 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 <unsigned int>
> >> [(struct real_valueD.28367 *)r_77(D)] ])
> >>>>>> (mem:SI (plus:SI (reg/v/f:SI 273 [ rD.73757 ])
> >>>>>> (const_int 20 [0x14])) [0 MEM <charD.7[1:24]>
> >> [(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 <charD.7[1:24]> [(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 <charD.7[1:24]> [(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
> >> <char[1:24]> [(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 <char[1:24]> [(void *)r_77(D)]+20 S4 A8])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> >> (const_int 16 [0x10])) [0 MEM <char[1:24]> [(void *)r_77(D)]+16 S4 A8])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> >> (const_int 12 [0xc])) [0 MEM <char[1:24]> [(void *)r_77(D)]+12 S4 A8])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> >> (const_int 8 [0x8])) [0 MEM <char[1:24]> [(void *)r_77(D)]+8 S4 A8])
> >> (mem:SI (plus:SI (reg/v/f:SI 273 [ r ])
> >> (const_int 4 [0x4])) [0 MEM <char[1:24]> [(void *)r_77(D)]+4 S4 A8])
> >> (mem:SI (reg/v/f:SI 273 [ r ]) [0 MEM <char[1:24]> [(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 <char[1:24]> [(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 <char[1:24]> [(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 and have no influence on the value itself.
> > For expression replacement you usually don't want expression equality but
> > other checks (even the for_gcse ones show lack of distinction here).
> >
> >>> 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 be better off loading the data into a REG, then
> >>> using the REG for the source of both memory stores.
> >>>
> >>
> >> I think Richi said, replacing MEM with a REG or a CONST would make
> >> sense,
> >> We could just try not replace MEM with different MEM.
> >
> > Yes. We can only replace expressions with ones that are obviously safe to
> > insert here. MEMs are not obviously safe (some might even trap). Regs and
> > constants are.
> >
>
> Hmm, yes, I can just ignore any misaligned MEMs in the equivalence class.
> This seems to fix the test case as it seems.
>
> Are we sure that replacing a read from a different alias set cannot cause
> problems here? I almost start to think so...
Sure, those will be a problem as well.
>
> So, how about this?
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. */
;
isn't better? I see we detect noop MEM = MEM earlier thus this kind
of check would need to be done after that. And note that code
is also buggy since it doesn't check for the case where the
store changes the dynamic type of the memory...
Index: cse.c
===================================================================
--- cse.c (revision 275648)
+++ cse.c (working copy)
@@ -5385,6 +5385,14 @@ cse_insn (rtx_insn *insn)
/* Do nothing for this case. */
;
+ else if (MEM_P (trial)
+ && MEM_P (SET_SRC (sets[i].rtl))
+ && !mem_attrs_eq_p (MEM_ATTRS (trial),
+ MEM_ATTRS (SET_SRC (sets[i].rtl))))
+ /* Do not try to replace a MEM with another MEM when
+ attributes like alignment and alias-set do not match. */
+ ;
+
/* Look for a substitution that makes a valid insn. */
else if (validate_unshare_change (insn, &SET_SRC (sets[i].rtl),
trial, 0))
