[Bug c/106912] New: [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032

[Bug c/106912] New: [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032

[gscfq@t-online.de]

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

            Bug ID: 106912
           Summary: [13 Regression] ICE in vect_transform_loops, at
                    tree-vectorizer.cc:1032
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gscfq@t-online.de
  Target Milestone: ---

Started between 20220703 and 20220710, at -O1+,
with file gcc.target/aarch64/simd_pcs_attribute-3.c :


$ cat z1.c
__attribute__ ((__simd__))
__attribute__ ((__nothrow__ , __leaf__ , __const__))
double foo (double x);
void bar(double *f, int n)
{
  int i;
  for (i = 0; i < n; i++)
    f[i] = foo(f[i]);
}
double foo(double x)
{
  return x * x / 3.0;
}


$ gcc-13-20220911 -c z1.c -O2 -fPIC -ftree-vectorize -fprofile-generate
during GIMPLE pass: vect
z1.c: In function 'bar':
z1.c:4:6: internal compiler error: in vect_transform_loops, at
tree-vectorizer.cc:1032
    4 | void bar(double *f, int n)
      |      ^~~
0xf5afe7 vect_transform_loops
        ../../gcc/tree-vectorizer.cc:1032
0xf5b394 try_vectorize_loop_1
        ../../gcc/tree-vectorizer.cc:1153
0xf5b394 try_vectorize_loop
        ../../gcc/tree-vectorizer.cc:1185
0xf5b904 execute
        ../../gcc/tree-vectorizer.cc:1299

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |marxin at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-09-12
             Status|UNCONFIRMED                 |NEW
            Summary|[13 Regression] ICE in      |[13 Regression] ICE in
                   |vect_transform_loops, at    |vect_transform_loops, at
                   |tree-vectorizer.cc:1032     |tree-vectorizer.cc:1032
                   |                            |since
                   |                            |r13-1575-gcf3a120084e94614

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with r13-1575-gcf3a120084e94614.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org
             Status|NEW                         |ASSIGNED
           Priority|P3                          |P1
   Target Milestone|---                         |13.0
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed, we have

# .MEM = VDEF <.MEM>
vect__5.57_58 = foo.simdclone.0 (vect__4.56_57);

here.  IIRC I filed a bugreport about simdclones not being const when the
scalar version is, in this case it's possibly IPA pure const not updating
the clones before materializing them!?

That said, the not vectorized variant is just

  _5 = foo (_4);

and without -fprofile-generate the vectorized variant also keeps 'const'.

I will look at this again after Cauldron.  Have to dig to where the
simdclone is actually generated.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
OK, it's a late IPA pass doing the clones it seems.  The scalar node got the
'const' stripped btw, but the call fntype still has it via the attributes.

It loses 'const' by

Old value = 252968993
New value = 251920417
set_const_flag_1 (node=<cgraph_node * 0x7ffff6530330 "foo"/1>, set_const=false,
looping=false, changed=0x7fffffffda5f) at
/home/rguenther/src/trunk/gcc/cgraph.cc:2696
2696              DECL_LOOPING_CONST_OR_PURE_P (node->decl) = false;
(gdb) bt
#0  set_const_flag_1 (node=<cgraph_node * 0x7ffff6530330 "foo"/1>, 
    set_const=false, looping=false, changed=0x7fffffffda5f)
    at /home/rguenther/src/trunk/gcc/cgraph.cc:2696
#1  0x0000000000da633f in cgraph_node::set_const_flag (
    this=<cgraph_node * const 0x7ffff6530330 "foo"/1>, set_const=false, 
    looping=false) at /home/rguenther/src/trunk/gcc/cgraph.cc:2789
#2  0x00000000015e2910 in tree_profiling ()
    at /home/rguenther/src/trunk/gcc/tree-profile.cc:818
#3  0x00000000015e2b9f in (anonymous namespace)::pass_ipa_tree_profile::execute
    (this=0x42a0c70) at /home/rguenther/src/trunk/gcc/tree-profile.cc:888

but the IL happily continues to treat the calls as 'const' because
flags_from_decl_or_type on the call fntype has

849       else if (TYPE_P (exp))
850         {
851           if (TYPE_READONLY (exp))
852             flags |= ECF_CONST;

note that __attribute__((pure)) is not duplicated on the type and so the
IPA profile effect will change the IL in fixup_cfg (), rewriting virtual
operands there.
making things consistent.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
static tree
handle_pure_attribute (tree *node, tree name, tree ARG_UNUSED (args),
                       int ARG_UNUSED (flags), bool *no_add_attrs)
{
  if (TREE_CODE (*node) == FUNCTION_DECL)
    {
      tree type = TREE_TYPE (*node);
      if (VOID_TYPE_P (TREE_TYPE (type)))
        warning (OPT_Wattributes, "%qE attribute on function "
                 "returning %<void%>", name);

      DECL_PURE_P (*node) = 1;
      /* ??? TODO: Support types.  */
    }
  else
    {
      warning (OPT_Wattributes, "%qE attribute ignored", name);
      *no_add_attrs = true;
    }

meh.

I also question that we clear const/pure after instrumenting.  We don't
do that for other instrumenting and it's only done for functions with
definitions but not functions in other (profile instrumented) TUs.
Edge counters do not alias anyway so that leaves for example
__gcov_time_profiler_counter which is updated like

  # VUSE <.MEM_14>
  PROF_time_profile_8 = __gcov_time_profiler_counterD.2013;
  PROF_time_profile_9 = PROF_time_profile_8 + 1;
  # .MEM_15 = VDEF <.MEM_14>
  __gcov7.fooD.2016[0] = PROF_time_profile_9;
  # .MEM_16 = VDEF <.MEM_15>
  __gcov_time_profiler_counterD.2013 = PROF_time_profile_9;

the counter is public, external and static (eh?).  We do eventually IPA
inline profile instrumented functions, but at least the above seems to be
at function end only.

That said, the const clearing doesn't have any effect, only the pure one
has.

It's possible to "fixup" in the vectorizer here but that's a hack for the
general inconsistency.  The vectorizer can also build a const qualified
function type variant for the call fntype which might be a better fit but
then the simd clones use the exact same counters as the original function
which might or might not be intended ...

Honza - any comments on the pure/const difference for profile instrumented
code here?  Do we need to "fix" the const case or can we drop the
pure/const state changes?  It depends on that what the correct fix in
the vectorizer is.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> OK, it's a late IPA pass doing the clones it seems.  The scalar node got the
> 'const' stripped btw, but the call fntype still has it via the attributes.
> 
> It loses 'const' by
> 
> Old value = 252968993
> New value = 251920417
> set_const_flag_1 (node=<cgraph_node * 0x7ffff6530330 "foo"/1>,
> set_const=false, looping=false, changed=0x7fffffffda5f) at
> /home/rguenther/src/trunk/gcc/cgraph.cc:2696
> 2696              DECL_LOOPING_CONST_OR_PURE_P (node->decl) = false;
> (gdb) bt
> #0  set_const_flag_1 (node=<cgraph_node * 0x7ffff6530330 "foo"/1>, 
>     set_const=false, looping=false, changed=0x7fffffffda5f)
>     at /home/rguenther/src/trunk/gcc/cgraph.cc:2696
> #1  0x0000000000da633f in cgraph_node::set_const_flag (
>     this=<cgraph_node * const 0x7ffff6530330 "foo"/1>, set_const=false, 
>     looping=false) at /home/rguenther/src/trunk/gcc/cgraph.cc:2789
> #2  0x00000000015e2910 in tree_profiling ()
>     at /home/rguenther/src/trunk/gcc/tree-profile.cc:818
> #3  0x00000000015e2b9f in (anonymous
> namespace)::pass_ipa_tree_profile::execute
>     (this=0x42a0c70) at /home/rguenther/src/trunk/gcc/tree-profile.cc:888
> 
> but the IL happily continues to treat the calls as 'const' because
> flags_from_decl_or_type on the call fntype has
> 
> 849       else if (TYPE_P (exp))
> 850         {
> 851           if (TYPE_READONLY (exp))
> 852             flags |= ECF_CONST;
> 
> note that __attribute__((pure)) is not duplicated on the type and so the
> IPA profile effect will change the IL in fixup_cfg (), rewriting virtual
> operands there.
> making things consistent.

Just to note, since types are shared we cannot simply strip the attribute
from the function decls type.  If the instead unshare it and do the type
change local to the decl the calls to the function won't pick up this change.

So the only way out here is to never go const -> non-const (or pure ->
non-pure)
because that cannot work reliably (consider indirect calls for example).

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Sure, the FUNCTION_TYPE can be shared, so shouldn't be overwritten in place,
but can be copied with TREE_READONLY cleared on the copy.
So, if we for whatever reason need to clear const flag on some functions, after
clearing TREE_READONLY on the node->decl we'd also need to update the TREE_TYPE
to a version without TREE_READONLY set (so that say if new calls are created
they don't get wrong fntype) and then walk all call edges to that function,
updating gimple_call_fntype to the updated type if it was equal to the old
TREE_TYPE, or to a copy with TREE_READONLY cleared if it has TREE_READONLY set
on the FUNCTION/METHOD_TYPE).

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #6)
> Sure, the FUNCTION_TYPE can be shared, so shouldn't be overwritten in place,
> but can be copied with TREE_READONLY cleared on the copy.
> So, if we for whatever reason need to clear const flag on some functions,
> after clearing TREE_READONLY on the node->decl we'd also need to update the
> TREE_TYPE to a version without TREE_READONLY set (so that say if new calls
> are created they don't get wrong fntype) and then walk all call edges to
> that function, updating gimple_call_fntype to the updated type if it was
> equal to the old TREE_TYPE, or to a copy with TREE_READONLY cleared if it
> has TREE_READONLY set on the FUNCTION/METHOD_TYPE).

That will not catch indirect calls to the function.  So whatever we do
this part of the problem will remain (the out-of-sync profile).  Like

inline int __attribute__((const))
foo (int i)
{
  if (i == 0)
    return 2;
  return 1;
}

int (* __attribute__((const)) bar) () = foo;

int main()
{
  int r = 0;
  for (int i = 0; i < 10000; ++i)
    {
      r += foo (r);
      r += bar (r);
    }
  return r;
}

which, when compiled with -O2 -fno-early-inlining -fprofile-arcs results
in the edge counters from the inline copy via foo to be store-motion'ed
but the indirect call clobbering them each iteration.

Using -fprofile-use prevents this because we get an indirect call profiler
call which clobbers all memory, confusing LIM, doing early inlining will
early inline the function before profile instrumentation.

I cannot see how we can fix this without some global flag to ignore
const/pure-ness of functions or alternatively dropping DECL_NONALIASED.
Trying to support surgical un-setting of pure/const for single functions
isn't going to work.  Simply disabling IPA inlining for instrumented
builds might also work.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[pinskia at gcc dot gnu.org]

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

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think PR 106433 is the same exact issue.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #8)
> I think PR 106433 is the same exact issue.

It is not.  The PR106433 patch doesn't fix this, the bug there is late
set_const_flag, while in this case it is an early one.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Also, the other PR is about a function being made const late when it previously
wasn't, while in this case it is being made non-const after it has been const
before.
  /* Drop pure/const flags from instrumented functions.  */
  if (profile_arc_flag || flag_test_coverage)
    FOR_EACH_DEFINED_FUNCTION (node)
      {
        if (!gimple_has_body_p (node->decl)
            || !(!node->clone_of
            || node->decl != node->clone_of->decl))
          continue;

        /* Don't profile functions produced for builtin stuff.  */
        if (DECL_SOURCE_LOCATION (node->decl) == BUILTINS_LOCATION)
          continue;

        node->set_const_flag (false, false);
        node->set_pure_flag (false, false);
      }
just looks completely wrong to me for const, which unlike pure attribute can be
on both function types and on function declarations.  If we want calls to const
functions to actually not be treated as const when profiling, it can be only
done by ignoring const everywhere if profile_arc_flag || flag_test_coverage. 
It even can't be just ignored for direct calls to defined functions, because
calls can be indirect at first, without vops, and then fndecl propagated and
turned into direct.  Or for IPA they can be defined in other TUs.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
See https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607202.html and the
reaction from Honza.(In reply to Jakub Jelinek from comment #10)
> Also, the other PR is about a function being made const late when it
> previously wasn't, while in this case it is being made non-const after it
> has been const before.
>   /* Drop pure/const flags from instrumented functions.  */
>   if (profile_arc_flag || flag_test_coverage)
>     FOR_EACH_DEFINED_FUNCTION (node)
>       {
>         if (!gimple_has_body_p (node->decl)
>             || !(!node->clone_of
>             || node->decl != node->clone_of->decl))
>           continue;
> 
>         /* Don't profile functions produced for builtin stuff.  */
>         if (DECL_SOURCE_LOCATION (node->decl) == BUILTINS_LOCATION)
>           continue;
> 
>         node->set_const_flag (false, false);
>         node->set_pure_flag (false, false);
>       }
> just looks completely wrong to me for const, which unlike pure attribute can
> be on both function types and on function declarations.  If we want calls to
> const functions to actually not be treated as const when profiling, it can
> be only done by ignoring const everywhere if profile_arc_flag ||
> flag_test_coverage.  It even can't be just ignored for direct calls to
> defined functions, because calls can be indirect at first, without vops, and
> then fndecl propagated and turned into direct.  Or for IPA they can be
> defined in other TUs.

See https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607202.html and the
reaction from Honza.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[jakub at gcc dot gnu.org]

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614071.html
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614070.html

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[cvs-commit at gcc dot gnu.org]

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

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

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

commit r13-6849-ged626f18b189920aeed9974aded3f9cb6f25b543
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Mar 16 13:51:19 2023 +0100

    tree-optimization/106912 - clear const attribute from fntype

    The following makes sure that after clearing pure/const from
    instrumented function declarations we are adjusting call statements
    fntype as well to handle indirect calls and because gimple_call_flags
    looks at both decl and fntype.

    Like the pure/const flag clearing on decls we refrain from touching
    calls to known functions that do not have a body in the current TU.

            PR tree-optimization/106912
            * tree-profile.cc (tree_profiling): Update stmts only when
            profiling or testing coverage.  Make sure to update calls
            fntype, stripping 'const' there.

            * gcc.dg/profile-generate-4.c: New testcase.

[Bug tree-optimization/106912] [13 Regression] ICE in vect_transform_loops, at tree-vectorizer.cc:1032 since r13-1575-gcf3a120084e94614

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed, but possibly latent.