Re: fix dse check for read/group conflict

[Richard Guenther <richard dot guenther at gmail dot com>]

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.
>