On Mon, 4 Apr 2016, Richard Biener wrote:
> On Fri, 1 Apr 2016, Bernd Schmidt wrote:
>
> > On 04/01/2016 11:08 AM, Richard Biener wrote:
> > > {
> > > ! if (canon_true_dependence (s_info->mem,
> > > ! GET_MODE (s_info->mem),
> > > ! s_info->mem_addr,
> > > ! mem, mem_addr))
> > > {
> > > s_info->rhs = NULL;
> > > s_info->const_rhs = NULL;
> > > --- 1609,1617 ----
> > > the value of store_info. If it is, set the rhs to NULL to
> > > keep it from being used to remove a load. */
> > > {
> > > ! if (canon_output_dependence (s_info->mem, true,
> > > ! mem, GET_MODE (mem),
> > > ! mem_addr))
> > > {
> > > s_info->rhs = NULL;
> > > s_info->const_rhs = NULL;
> >
> > I think the patch is ok, but there is a comment in that function which
> > references canon_true_dependence; that should also be fixed up.
>
> Done, though I don't understand it at all ... if alias-set was supposed
> to be zero for all cases we call canon_true_dependence then the issue
> wouldn't have happened. Maybe there was times where passing mem_addr
> == NULL_RTX to canon_true_dependence caused it to bail out?
>
> Not sure how to adjust that comment now, maybe it would be valid
> to simply remove the if (spill_alias_set) case and always use
> the else case?
>
> > Isn't the testcase invalid though? I thought accesses through char *
> > pointers bypass aliasing rules, but accessing a char array through int *
> > and long * pointers doesn't?
>
> I have installed it as the following with adjusted testcase, if somebody
> can shed some light on the odd comment I'll happily followup.
Jakub added that comment in r146669. Still the code only handles
alias-sets being different specially by not deleting such stores.
But it doesn't break from the loop there (if that store is aliasing).
I suppose this code might have similar issues for deleting "dead"
stores when not replacing the RHS.
Richard.
> Richard.
>
> 2016-04-04 Richard Biener <rguent...@suse.de>
>
> PR rtl-optimization/70484
> * rtl.h (canon_output_dependence): Declare.
> * alias.c (canon_output_dependence): New function.
> * dse.c (record_store): Use canon_output_dependence rather
> than canon_true_dependence.
>
> * gcc.dg/torture/pr70484.c: New testcase.
>
> Index: gcc/rtl.h
> ===================================================================
> *** gcc/rtl.h (revision 234663)
> --- gcc/rtl.h (working copy)
> *************** extern int anti_dependence (const_rtx, c
> *** 3652,3657 ****
> --- 3652,3659 ----
> extern int canon_anti_dependence (const_rtx, bool,
> const_rtx, machine_mode, rtx);
> extern int output_dependence (const_rtx, const_rtx);
> + extern int canon_output_dependence (const_rtx, bool,
> + const_rtx, machine_mode, rtx);
> extern int may_alias_p (const_rtx, const_rtx);
> extern void init_alias_target (void);
> extern void init_alias_analysis (void);
> Index: gcc/alias.c
> ===================================================================
> *** gcc/alias.c (revision 234663)
> --- gcc/alias.c (working copy)
> *************** output_dependence (const_rtx mem, const_
> *** 3057,3062 ****
> --- 3057,3076 ----
> /*mem_canonicalized=*/false,
> /*x_canonicalized*/false, /*writep=*/true);
> }
> +
> + /* Likewise, but we already have a canonicalized MEM, and X_ADDR for X.
> + Also, consider X in X_MODE (which might be from an enclosing
> + STRICT_LOW_PART / ZERO_EXTRACT).
> + If MEM_CANONICALIZED is true, MEM is canonicalized. */
> +
> + int
> + canon_output_dependence (const_rtx mem, bool mem_canonicalized,
> + const_rtx x, machine_mode x_mode, rtx x_addr)
> + {
> + return write_dependence_p (mem, x, x_mode, x_addr,
> + mem_canonicalized, /*x_canonicalized=*/true,
> + /*writep=*/true);
> + }
>
>
>
> Index: gcc/dse.c
> ===================================================================
> *** gcc/dse.c (revision 234663)
> --- gcc/dse.c (working copy)
> *************** record_store (rtx body, bb_info_t bb_inf
> *** 1609,1618 ****
> the value of store_info. If it is, set the rhs to NULL to
> keep it from being used to remove a load. */
> {
> ! if (canon_true_dependence (s_info->mem,
> ! GET_MODE (s_info->mem),
> ! s_info->mem_addr,
> ! mem, mem_addr))
> {
> s_info->rhs = NULL;
> s_info->const_rhs = NULL;
> --- 1609,1617 ----
> the value of store_info. If it is, set the rhs to NULL to
> keep it from being used to remove a load. */
> {
> ! if (canon_output_dependence (s_info->mem, true,
> ! mem, GET_MODE (mem),
> ! mem_addr))
> {
> s_info->rhs = NULL;
> s_info->const_rhs = NULL;
> Index: gcc/testsuite/gcc.dg/torture/pr70484.c
> ===================================================================
> *** gcc/testsuite/gcc.dg/torture/pr70484.c (revision 0)
> --- gcc/testsuite/gcc.dg/torture/pr70484.c (revision 0)
> ***************
> *** 0 ****
> --- 1,19 ----
> + /* { dg-do run } */
> +
> + extern void abort (void);
> +
> + int __attribute__((noinline,noclone))
> + f(int *pi, long *pl)
> + {
> + *pi = 1;
> + *pl = 0;
> + return *(char *)pi;
> + }
> +
> + int main()
> + {
> + union { long l; int i; } a;
> + if (f (&a.i, &a.l) != 0)
> + abort ();
> + return 0;
> + }
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
