From patchwork Fri Aug 27 10:08:02 2010
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: fix dse check for read/group conflict
Date: Fri, 27 Aug 2010 00:08:02 -0000
From: Olivier Hainque <hainque@adacore.com>
X-Patchwork-Id: 62840
Message-Id: <20100827100802.GA16528@cardhu.act-europe.fr>
To: gcc-patches@gcc.gnu.org
Cc: hainque@adacore.com

Hello,

>From scan_reads_nospill, the RTL DSE check for read/group conflict goes like:

      <<          /* The groups are different, if the alias sets
                     conflict, clear the entire group.  We only need
                     to apply this test if the read_info is a cselib
                     read.  Anything with a constant base cannot alias
                     something else with a different constant
                     base.  */
                  if ((read_info->group_id < 0)
                      && canon_true_dependence (group->base_mem,
                                                QImode,
                                                group->canon_base_addr,
                                                read_info->mem, NULL_RTX,
                                                rtx_varies_p))
      >>

The use of QImode here is problematic because it shortens the group
memory span preceived by canon_true_dependence.

>From this we have witnessed silent wrong code generated (unduly removed store)
out of a gcc 4.3 based compiler for powerpc on the attached testcase,

The bug lead to missing a conflict between a group and a read, believed to
be non overlapping out of the memrefs_conflict_p test in true_dependence_1.

This doesn't reproduce identically on the testcase with mainline (different
insn set reaching here) but remains latent AFAICT.

The attached patch is suggestion to address this by making sure we use BLKmode
instead of QImode to extend the perceived group memory span. It also unifies
the way canon_true_dependence is called with other places, using GET_MODE on
the first mem to provide the mode argument.

It fixed the observable problem out of gcc 4.3 on powerpc and we have been
using it on many targets for a while without problem. We have been using it
successfully in 4.5 based runs as well.

It was also bootstrapped and regression tested on x86-suse-linux for mainline.

OK ?

Thanks in advance,

With Kind Regards,

Olivier

2010-08-27  Olivier Hainque  <hainque@adacore.com>
	    Eric Botcazou  <ebotcazou@adacore.com>

	* dse.c (group_info.base_mem, get_group_info): Use BLKmode to
	cover all the possible offsets from this base.
	(scan_reads_nospill): Pass base_mem's mode to canon_true_dependence.

	testsuite/
	* gnat.dg/dse_step.ads, dse_step.adb, test_dse_step.adb: New test.


Index: dse.c
===================================================================
--- dse.c	(revision 163446)
+++ dse.c	(working copy)
@@ -473,8 +473,9 @@
      hard_frame_pointer.  */
   bool frame_related;
 
-  /* A mem wrapped around the base pointer for the group in order to
-     do read dependency.  */
+  /* A mem wrapped around the base pointer for the group in order to do
+     read dependency.  It must be given BLKmode in order to encompass all
+     the possible offsets from the base.  */
   rtx base_mem;
 
   /* Canonized version of base_mem's address.  */
@@ -705,7 +706,7 @@
       *slot = gi = (group_info_t) pool_alloc (rtx_group_info_pool);
       gi->rtx_base = base;
       gi->id = rtx_group_next_id++;
-      gi->base_mem = gen_rtx_MEM (QImode, base);
+      gi->base_mem = gen_rtx_MEM (BLKmode, base);
       gi->canon_base_addr = canon_rtx (base);
       gi->store1_n = BITMAP_ALLOC (NULL);
       gi->store1_p = BITMAP_ALLOC (NULL);
@@ -3118,7 +3119,7 @@
 		     base.  */
 		  if ((read_info->group_id < 0)
 		      && canon_true_dependence (group->base_mem,
-						QImode,
+						GET_MODE (group->base_mem),
 						group->canon_base_addr,
 						read_info->mem, NULL_RTX,
 						rtx_varies_p))
Index: testsuite/gnat.dg/test_dse_step.adb
===================================================================
--- testsuite/gnat.dg/test_dse_step.adb	(revision 0)
+++ testsuite/gnat.dg/test_dse_step.adb	(revision 0)
@@ -0,0 +1,14 @@
+-- { dg-do compile }
+-- { dg-options "-O1 -gnatp -gnatn" }
+
+with Dse_Step; use Dse_Step;
+
+procedure Test_Dse_Step is
+   Start : My_Counter := (Value => 0, Step => 1);
+   Steps : Natural := Nsteps;
+begin
+   Step_From (Start);
+   if Mv /= Steps then
+      raise Program_Error;
+   end if;
+end;
Index: testsuite/gnat.dg/dse_step.adb
===================================================================
--- testsuite/gnat.dg/dse_step.adb	(revision 0)
+++ testsuite/gnat.dg/dse_step.adb	(revision 0)
@@ -0,0 +1,18 @@
+package body Dse_Step is
+
+   procedure Do_Step (This : in out Counter) is
+   begin
+      This.Value := This.Value + This.Step;
+   end;
+
+   procedure Step_From (Start : in My_Counter) is
+      Lc : My_Counter := Start;
+   begin
+      while Nsteps > 0 loop
+         Do_Step (Lc);
+         Nsteps := Nsteps - 1;
+      end loop;
+      Mv := Lc.Value;
+   end;
+
+end;
Index: testsuite/gnat.dg/dse_step.ads
===================================================================
--- testsuite/gnat.dg/dse_step.ads	(revision 0)
+++ testsuite/gnat.dg/dse_step.ads	(revision 0)
@@ -0,0 +1,19 @@
+package Dse_Step is
+
+   type Counter is record
+      Value : Natural;
+      Step  : Natural;
+   end record;
+   pragma Suppress_Initialization (Counter);
+
+   procedure Do_Step (This : in out Counter);
+   pragma Inline (Do_Step);
+
+   type My_Counter is new Counter;
+   pragma Suppress_Initialization (My_Counter);
+
+   procedure Step_From (Start : in My_Counter);
+
+   Nsteps : Natural := 12;
+   Mv : Natural;
+end;