[Bug rtl-optimization/60819] New: dse1 removing not-dead insn (aliasing issue?)

[Bug rtl-optimization/60819] New: dse1 removing not-dead insn (aliasing issue?)

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

            Bug ID: 60819
           Summary: dse1 removing not-dead insn (aliasing issue?)
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: glisse at gcc dot gnu.org
            Target: x86_64-linux-gnu

#include <mmintrin.h>

int f (__m64 const __A, int const __N)
{ return ((__v4hi)__A)[__N]; }


compiler with -O2, produces:

    movslq    %edi, %rdi
    movswl    -8(%rsp,%rdi,2), %eax
    ret

dse1 removed the instruction that copies xmm0 to the stack!

Could be related to PR 56965, I didn't try the patch in that PR.
Already broken in gcc-4.6, not a (recent) regression.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-04-11
          Component|rtl-optimization            |c
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think you are violating aliasing in your testcase:

typedef int __m64 __attribute__ ((__vector_size__ (8), __may_alias__));
typedef short __v4hi __attribute__ ((__vector_size__ (8)));

you access __m64 with a __v4hi (__may_alias__ doesn't make the __m64 parameter
have alias-set zero!).

Now, we lower this to

  return (int) *((short int *) &VIEW_CONVERT_EXPR<vector(4) short int>(__A) +
(sizetype) ((long unsigned int) __N * 2));

because of the variable indexing which makes this a memory access.  So
eventually that lowering should make sure no TBAA issues appear or we
should reject variable indexes here.

That is, variable indexing should use a MEM_REF with alias-set zero
(if we are sure that non-variable indexing will never result in a memory
access,
it seems that the FE lowers constant indexes the same and only later during
gimplification we fold that to use a BIT_FIELD_REF).

Confirmed as a Frontend bug sofar, just in case the testcase is valid.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Btw, eventually better representation:

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c     (revision 209298)
+++ gcc/c-family/c-common.c     (working copy)
@@ -11760,20 +11760,18 @@ convert_vector_to_pointer_for_subscript
 {
   if (TREE_CODE (TREE_TYPE (*vecp)) == VECTOR_TYPE)
     {
-      tree type = TREE_TYPE (*vecp);
-      tree type1;
+      tree vtype = TREE_TYPE (*vecp);
+      tree type;

       if (TREE_CODE (index) == INTEGER_CST)
         if (!tree_fits_uhwi_p (index)
-            || tree_to_uhwi (index) >= TYPE_VECTOR_SUBPARTS (type))
+            || tree_to_uhwi (index) >= TYPE_VECTOR_SUBPARTS (vtype))
           warning_at (loc, OPT_Warray_bounds, "index value is out of bound");

       c_common_mark_addressable_vec (*vecp);
-      type = build_qualified_type (TREE_TYPE (type), TYPE_QUALS (type));
-      type = build_pointer_type (type);
-      type1 = build_pointer_type (TREE_TYPE (*vecp));
-      *vecp = build1 (ADDR_EXPR, type1, *vecp);
-      *vecp = convert (type, *vecp);
+      type = build_qualified_type (TREE_TYPE (vtype), TYPE_QUALS (vtype));
+      type = build_array_type_nelts (type, TYPE_VECTOR_SUBPARTS (vtype));
+      *vecp = build1 (VIEW_CONVERT_EXPR, type, *vecp);
     }
 }

which defers the "issue" to RTL expansion, expanding from

  _3 = VIEW_CONVERT_EXPR<short int[4]>(__A_5(D))[__N_2(D)];
  _4 = (int) _3;
  return _4;

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Btw, we don't simplify

  _2 = VIEW_CONVERT_EXPR<short int[4]>(__A_4(D))[2];

to a BIT_FIELD_REF.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
But it shows that expansion when falling back to the stack doesn't properly
consider TBAA.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #5 from Marc Glisse <glisse at gcc dot gnu.org> ---
typedef int v2si __attribute__((vector_size (8),may_alias));
typedef short v4hi __attribute__((vector_size (8),may_alias));
int f (v2si __A, int __N)
{ return (*(v4hi*)&__A)[__N]; }

also fails.

Reading the documentation of may_alias, I have the impression that this example
is doing exactly what may_alias is supposed to allow, no?

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #6 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 11 Apr 2014, glisse at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819
> 
> --- Comment #5 from Marc Glisse <glisse at gcc dot gnu.org> ---
> typedef int v2si __attribute__((vector_size (8),may_alias));
> typedef short v4hi __attribute__((vector_size (8),may_alias));
> int f (v2si __A, int __N)
> { return (*(v4hi*)&__A)[__N]; }
> 
> also fails.
> 
> Reading the documentation of may_alias, I have the impression that this example
> is doing exactly what may_alias is supposed to allow, no?

Not exactly - it's again the wrapping that will fail.

typedef int aint __attribute__((may_alias));

return ((aint*)&__A)[__N];

would make it valid.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
I like the VCE using an array-type representation more because that won't
make the thing addressable early on (or force it to be alias-set zero).  We
still have to fix expansion, of course.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #8 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #6)
> Not exactly - it's again the wrapping that will fail.

That seems strange to me. From the doc of may_alias:

"Accesses through pointers to types with this attribute are not subject to
type-based alias analysis, but are instead assumed to be able to alias any
other type of objects." "This extension exists to support some vector APIs, in
which pointers to one vector type are permitted to alias pointers to a
different vector type."

and indeed the x86 *intrin.h files cast between __m128i and __v4si (and many
other combinations) all the time.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #9 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 11 Apr 2014, glisse at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819
> 
> --- Comment #8 from Marc Glisse <glisse at gcc dot gnu.org> ---
> (In reply to rguenther@suse.de from comment #6)
> > Not exactly - it's again the wrapping that will fail.
> 
> That seems strange to me. From the doc of may_alias:
> 
> "Accesses through pointers to types with this attribute are not subject to
> type-based alias analysis, but are instead assumed to be able to alias any
> other type of objects." "This extension exists to support some vector APIs, in
> which pointers to one vector type are permitted to alias pointers to a
> different vector type."
> 
> and indeed the x86 *intrin.h files cast between __m128i and __v4si (and many
> other combinations) all the time.

But the actual access will be that generated by the frontend via a
pointer to a scalar (that doesn't have the ref-all flag set).

Patch that should make your 2nd testcase work as expected:

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c     (revision 209298)
+++ gcc/c-family/c-common.c     (working copy)
@@ -11770,8 +11770,9 @@ convert_vector_to_pointer_for_subscript

       c_common_mark_addressable_vec (*vecp);
       type = build_qualified_type (TREE_TYPE (type), TYPE_QUALS (type));
-      type = build_pointer_type (type);
       type1 = build_pointer_type (TREE_TYPE (*vecp));
+      type = build_pointer_type_for_mode (type, ptr_mode,
+                                         TYPE_REF_CAN_ALIAS_ALL (type1));
       *vecp = build1 (ADDR_EXPR, type1, *vecp);
       *vecp = convert (type, *vecp);
     }

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #10 from Marc Glisse <glisse at gcc dot gnu.org> ---
Ah. So, from a user's POV, it should be valid. But internally, we convert
v2si*->v4hi* (ok), v4hi*->short* (ok), we forget the intermediate step, and we
end up with v2si*->short*, which is not ok since the type through which the
access will happen (short) doesn't have may_alias. The safety of the cast is
not transitive...

Maybe the patch from comment #9 could be restricted to the case where the
vector type has may_alias? That would still break my original testcase, but it
seems illegal anyway (may_alias only works in one direction, the opposite one,
and I think the x86 *intrin.h files are wrong).

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #11 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #10)
> Maybe the patch from comment #9 could be restricted to the case where the
> vector type has may_alias?

Oups, that's already the case, sorry for misreading.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #12 from Marc Glisse <glisse at gcc dot gnu.org> ---
Created attachment 32586
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32586&action=edit
Richard's small patch

I regtested it (patch from comment #9 plus a testcase) on x86_64-linux-gnu and
it passed. I think we should go with this in the short term. Would you care to
handle it? Or should I post it for review by Joseph/Jason?

The array-based expansion (for later) would require the same may_alias
treatment. may_alias morally belongs to TYPE_QUALS (like const/volatile, it is
safe to add, not to remove) and it is logical to propagate it in the same
place.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
      Known to work|                            |4.10.0
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed on trunk sofar.

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

--- Comment #16 from Richard Biener <rguenth at gcc dot gnu.org> ---
Author: rguenth
Date: Tue Apr 22 13:26:02 2014
New Revision: 209629

URL: http://gcc.gnu.org/viewcvs?rev=209629&root=gcc&view=rev
Log:
2014-04-22  Richard Biener  <rguenther@suse.de>

    Backport from mainline
    2014-04-14  Richard Biener  <rguenther@suse.de>
        Marc Glisse  <marc.glisse@inria.fr>

    PR c/60819
    * c-common.c (convert_vector_to_pointer_for_subscript): Properly
    apply may-alias the scalar pointer type when applicable.

    * gcc.target/i386/vec-may_alias.c: New testcase.

Added:
    branches/gcc-4_9-branch/gcc/testsuite/gcc.target/i386/vec-may_alias.c
Modified:
    branches/gcc-4_9-branch/gcc/c-family/ChangeLog
    branches/gcc-4_9-branch/gcc/c-family/c-common.c
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog

[Bug c/60819] dse1 removing not-dead insn (aliasing issue?)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60819

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |4.9.1

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.