| Message ID | 20100827100802.GA16528@cardhu.act-europe.fr |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 27, 2010 at 12:08 PM, Olivier Hainque <hainque@adacore.com> wrote: > 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 ? Ok. Thanks, Richard. > 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. >
Richard Guenther wrote:
> Ok.
Thanks for the super-fast review :)
Olivier
On Fri, Aug 27, 2010 at 12:47 PM, Olivier Hainque <hainque@adacore.com> wrote: > Richard Guenther wrote: >> Ok. > > Thanks for the super-fast review :) Happens when you look for an excuse not to work ;) Richard. > Olivier >
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;
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.