[Bug c/99517] New: __builtin_convertvector with casts

[Bug c/99517] New: __builtin_convertvector with casts

[me+gcc.gnu at lelf dot lu]

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

            Bug ID: 99517
           Summary: __builtin_convertvector with casts
           Product: gcc
           Version: 10.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: me+gcc.gnu at lelf dot lu
  Target Milestone: ---

#include<stdint.h>

typedef int8_t  GV __attribute__((vector_size(32),aligned(1)));

typedef int32_t IV __attribute__((vector_size(16),aligned(1)));
typedef int64_t JV __attribute__((vector_size(32),aligned(1)));
typedef double  FV __attribute__((vector_size(32),aligned(1)));

GV FI(IV x){return(GV)__builtin_convertvector(x,FV);}
GV JI(IV x){return(GV)__builtin_convertvector(x,JV);}


x86_64
-Os or -O2
$ gcc-11x -Os -c cv.c -mavx2


FI is ok

0000000000000000 <FI>:
   0:   c5 fa e6 c8             vcvtdq2pd %xmm0,%xmm1
   4:   c5 f9 70 c0 ee          vpshufd $0xee,%xmm0,%xmm0
   9:   c5 fa e6 c0             vcvtdq2pd %xmm0,%xmm0
   d:   c4 e3 75 18 c0 01       vinsertf128 $0x1,%xmm0,%ymm1,%ymm0
  13:   c3                      retq   


but JI is wrong (body of FI)

0000000000000014 <JI>:
  14:   c5 fa e6 c8             vcvtdq2pd %xmm0,%xmm1
  18:   c5 f9 70 c0 ee          vpshufd $0xee,%xmm0,%xmm0
  1d:   c5 fa e6 c0             vcvtdq2pd %xmm0,%xmm0
  21:   c4 e3 75 18 c0 01       vinsertf128 $0x1,%xmm0,%ymm1,%ymm0
  27:   c3                      retq   




versions:
gcc-10x (GCC) 10.2.1 20210227
gcc-11x (GCC) 11.0.1 20210228 (experimental)

[Bug c/99517] __builtin_convertvector with casts

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-03-10
           Keywords|                            |wrong-code

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Both functions get the same IL, the convertvector destination type is lost.

;; Function FI (null)
;; enabled by -tree-original


{
  return VIEW_CONVERT_EXPR<GV>(.VEC_CONVERT (x));
}


;; Function JI (null)
;; enabled by -tree-original


{
  return VIEW_CONVERT_EXPR<GV>(.VEC_CONVERT (x));
}

[Bug ipa/99517] __builtin_convertvector with casts

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
             Status|NEW                         |ASSIGNED
          Component|c                           |ipa

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The bug is in IPA-ICF, testing a fix.

[Bug ipa/99517] __builtin_convertvector with casts

[marxin at gcc dot gnu.org]

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

--- Comment #3 from Martin Liška <marxin at gcc dot gnu.org> ---
Heh, one another ICF issue. I thought we already fixed all of them :)

[Bug ipa/99517] __builtin_convertvector with casts

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
--- gcc/ipa-icf-gimple.c.jj     2021-01-04 10:25:38.752234741 +0100
+++ gcc/ipa-icf-gimple.c        2021-03-10 15:02:06.287502784 +0100
@@ -667,7 +667,7 @@ func_checker::compare_gimple_call (gcall
   tree fntype1 = gimple_call_fntype (s1);
   tree fntype2 = gimple_call_fntype (s2);

-  /* For direct calls we verify that types are comopatible so if we matced
+  /* For direct calls we verify that types are compatible so if we matched
      callees, callers must match, too.  For indirect calls however verify
      function type.  */
   if (!gimple_call_fndecl (s1))
@@ -703,6 +703,14 @@ func_checker::compare_gimple_call (gcall
   t1 = gimple_get_lhs (s1);
   t2 = gimple_get_lhs (s2);

+  /* For internal calls, lhs types need to be verified, as neither fntype nor
+     callee comparisions can catch that.  */
+  if (gimple_call_internal_p (s1)
+      && t1
+      && t2
+      && !compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return return_false_with_msg ("GIMPLE internal call LHS type mismatch");
+
   return compare_operand (t1, t2, get_operand_access_type (&map, t1));
 }


fixes this for me, working on the testcase for testsuite now.

Anyway, I'm a little bit worried that gimple_call_fntype isn't compared for
direct calls, gimple_call_fntype doesn't have to match the function type of
gimple_call_fndecl.
And also surprised that compare_ssa_name e.g. doesn't compare
SSA_NAME_OCCURS_IN_ABNORMAL_PHI or SSA_NAME_PTR_INFO or SSA_NAME_RANGE_INFO.
Maybe for the latter two we are fine if we only optimize away
__builtin_unreachable calls after IPA, but if we do it before, e.g. some
function could have if (cond) __builtin_unreachable (); assertions that EVRP
would stick into SSA_NAME_RANGE_INFO and another function be the same except
without those assertions.  If ICF merges the latter into the former and calls
the latter with arguments violating the assertions of the former, it might be
miscompiled.

[Bug ipa/99517] __builtin_convertvector with casts

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50353
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50353&action=edit
gcc11-pr99517.patch

Untested fix.

[Bug ipa/99517] __builtin_convertvector with casts

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #4)
> --- gcc/ipa-icf-gimple.c.jj	2021-01-04 10:25:38.752234741 +0100
> +++ gcc/ipa-icf-gimple.c	2021-03-10 15:02:06.287502784 +0100
> @@ -667,7 +667,7 @@ func_checker::compare_gimple_call (gcall
>    tree fntype1 = gimple_call_fntype (s1);
>    tree fntype2 = gimple_call_fntype (s2);
>  
> -  /* For direct calls we verify that types are comopatible so if we matced
> +  /* For direct calls we verify that types are compatible so if we matched
>       callees, callers must match, too.  For indirect calls however verify
>       function type.  */
>    if (!gimple_call_fndecl (s1))
> @@ -703,6 +703,14 @@ func_checker::compare_gimple_call (gcall
>    t1 = gimple_get_lhs (s1);
>    t2 = gimple_get_lhs (s2);
>  
> +  /* For internal calls, lhs types need to be verified, as neither fntype
> nor
> +     callee comparisions can catch that.  */
> +  if (gimple_call_internal_p (s1)
> +      && t1
> +      && t2
> +      && !compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2)))
> +    return return_false_with_msg ("GIMPLE internal call LHS type mismatch");
> +
>    return compare_operand (t1, t2, get_operand_access_type (&map, t1));
>  }
>  
> 
> fixes this for me, working on the testcase for testsuite now.
> 
> Anyway, I'm a little bit worried that gimple_call_fntype isn't compared for
> direct calls, gimple_call_fntype doesn't have to match the function type of
> gimple_call_fndecl.

The comment says that we match that the fndecls definitions are equal,
one could argue that it's undefined behavior if the same definition is
invoked via two different gimple_call_fntype signatures and thus it's
OK to just go ahead (I think actual arguments are also compared so it
would collide there, too).

> And also surprised that compare_ssa_name e.g. doesn't compare
> SSA_NAME_OCCURS_IN_ABNORMAL_PHI or SSA_NAME_PTR_INFO or SSA_NAME_RANGE_INFO.
> Maybe for the latter two we are fine if we only optimize away
> __builtin_unreachable calls after IPA, but if we do it before, e.g. some
> function could have if (cond) __builtin_unreachable (); assertions that EVRP
> would stick into SSA_NAME_RANGE_INFO and another function be the same except
> without those assertions.  If ICF merges the latter into the former and
> calls the latter with arguments violating the assertions of the former, it
> might be miscompiled.

Hmm, yeah.  I guess one could try to carefully craft such a testcase.
For example non-null ranges can also be created from dead loads like

foo (int *p)
{
  *p;
  if (p != 0)
...

when you make sure to not run DCE before EVRP.  Not sure if that's enough
of a building-block to create a testcase.

[Bug ipa/99517] __builtin_convertvector with casts

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:070ab283d16d8e8e8bb70f9801aca347f008cbd0

commit r11-7618-g070ab283d16d8e8e8bb70f9801aca347f008cbd0
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Mar 11 10:59:18 2021 +0100

    icf: Check return type of internal fn calls [PR99517]

    The following testcase is miscompiled, because IPA-ICF considers the two
    functions identical.  They aren't, the types of the .VEC_CONVERT call
    lhs is different.  But for calls to internal functions, there is no
    fntype nor callee with a function type to compare, so all we compare
    is just the ifn, arguments and some call flags.

    The following patch fixes it by checking the internal fn calls like e.g.
gimple
    assignments where the type of the lhs is checked too.

    2021-03-11  Jakub Jelinek  <jakub@redhat.com>

            PR ipa/99517
            * ipa-icf-gimple.c (func_checker::compare_gimple_call): For
internal
            function calls with lhs fail if the lhs don't have compatible
types.

            * gcc.target/i386/avx2-pr99517-1.c: New test.
            * gcc.target/i386/avx2-pr99517-2.c: New test.

[Bug ipa/99517] [10 Regression] __builtin_convertvector with casts

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
            Summary|__builtin_convertvector     |[10 Regression]
                   |with casts                  |__builtin_convertvector
                   |                            |with casts
   Target Milestone|---                         |10.3

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
This is actually a regression that started with
r10-4643-ga37f58f506e436bdf8f4f5be4afbf2d246538058
Now fixed on the trunk.

[Bug ipa/99517] [10 Regression] __builtin_convertvector with casts

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:b0d1a533d62ff137e3d54ccf32cf876e5b49d2ab

commit r10-9483-gb0d1a533d62ff137e3d54ccf32cf876e5b49d2ab
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Mar 11 10:59:18 2021 +0100

    icf: Check return type of internal fn calls [PR99517]

    The following testcase is miscompiled, because IPA-ICF considers the two
    functions identical.  They aren't, the types of the .VEC_CONVERT call
    lhs is different.  But for calls to internal functions, there is no
    fntype nor callee with a function type to compare, so all we compare
    is just the ifn, arguments and some call flags.

    The following patch fixes it by checking the internal fn calls like e.g.
gimple
    assignments where the type of the lhs is checked too.

    2021-03-11  Jakub Jelinek  <jakub@redhat.com>

            PR ipa/99517
            * ipa-icf-gimple.c (func_checker::compare_gimple_call): For
internal
            function calls with lhs fail if the lhs don't have compatible
types.

            * gcc.target/i386/avx2-pr99517-1.c: New test.
            * gcc.target/i386/avx2-pr99517-2.c: New test.

    (cherry picked from commit 070ab283d16d8e8e8bb70f9801aca347f008cbd0)

[Bug ipa/99517] [10 Regression] __builtin_convertvector with casts

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 10.3 too.