[Bug tree-optimization/110531] New: Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[Bug tree-optimization/110531] New: Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[hliu at amperecomputing dot com]

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

            Bug ID: 110531
           Summary: Vect: slp_done_for_suggested_uf is not initialized in
                    tree-vect-loop.cc
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hliu at amperecomputing dot com
  Target Milestone: ---

This seems an obvious bug in tree-vect-loop.cc:

(1) This var is declared (but not initialized) and used in function
vect_analyze_loop_1:

  bool slp_done_for_suggested_uf;           <---- Warning, this is not
initialized

  /* Run the main analysis.  */
  opt_result res = vect_analyze_loop_2 (loop_vinfo, fatal,
                                        &suggested_unroll_factor,
                                        slp_done_for_suggested_uf);

(2) It is used before set in function vect_analyze_loop_2:
static opt_result
vect_analyze_loop_2 (loop_vec_info loop_vinfo, bool &fatal,
                     unsigned *suggested_unroll_factor,
                     bool& slp_done_for_suggested_uf)
  ...
  bool slp = !applying_suggested_uf || slp_done_for_suggested_uf;  <--- used
without initialized
  ...
  slp_done_for_suggested_uf = slp;


I don't know the detail logic and wonder if it should be initialized as "true"
or "false" (probably it should be "false").

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[linkw at gcc dot gnu.org]

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

Kewen Lin <linkw at gcc dot gnu.org> changed:

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

--- Comment #1 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Hao Liu from comment #0)
> This seems an obvious bug in tree-vect-loop.cc:
> 
> (1) This var is declared (but not initialized) and used in function
> vect_analyze_loop_1:
> 
>   bool slp_done_for_suggested_uf;           <---- Warning, this is not
> initialized
> 
>   /* Run the main analysis.  */
>   opt_result res = vect_analyze_loop_2 (loop_vinfo, fatal,
> 					&suggested_unroll_factor,
> 					slp_done_for_suggested_uf);
> 
> (2) It is used before set in function vect_analyze_loop_2:
> static opt_result
> vect_analyze_loop_2 (loop_vec_info loop_vinfo, bool &fatal,
> 		     unsigned *suggested_unroll_factor,
> 		     bool& slp_done_for_suggested_uf)
>   ...
>   bool slp = !applying_suggested_uf || slp_done_for_suggested_uf;  <--- used
> without initialized
>   ...
>   slp_done_for_suggested_uf = slp;
> 
> 
> I don't know the detail logic and wonder if it should be initialized as
> "true" or "false" (probably it should be "false").

Is the warning from some static analyzer?

This behavior was intentional, in the first time to call vect_analyze_loop_2,
we have loop_vinfo->suggested_unroll_factor = 1, applying_suggested_uf would be
false always, it means that at the below place

  bool slp = !applying_suggested_uf || slp_done_for_suggested_uf;

slp should be true always (always do analyze slp), it doesn't care what's in
slp_done_for_suggested_uf.

For a successful analysis, we would definitely do:

  slp_done_for_suggested_uf = slp;

So in the 2nd call to vect_analyze_loop_2, slp_done_for_suggested_uf should get
assigned by expected valu (should not be uninitialized).

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[hliu at amperecomputing dot com]

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

--- Comment #2 from Hao Liu <hliu at amperecomputing dot com> ---
> Is the warning from some static analyzer?

No. I just find it maybe a bug while looking at the code.

> slp should be true always (always do analyze slp), it doesn't care what's in slp_done_for_suggested_uf.

Oh, I see. This is not a real bug.


IMHO, it would be better to initialize it as "false", which should be much
easier for someone to understand the code.

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[linkw at gcc dot gnu.org]

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

--- Comment #3 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Hao Liu from comment #2)
> > Is the warning from some static analyzer?
> 
> No. I just find it maybe a bug while looking at the code.
> 
> > slp should be true always (always do analyze slp), it doesn't care what's in slp_done_for_suggested_uf.
> 
> Oh, I see. This is not a real bug.
> 
> 
> IMHO, it would be better to initialize it as "false", which should be much
> easier for someone to understand the code.

IMHO, the initialization with false is unnecessary and very likely it isn't
able to get optimized, it seems worse from this point of view. I was also
taught previously that useless initialization isn't recommended as sometimes it
can cover problems. But for the readability maybe one comment with more
explanation is worthy and better.

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[hliu at amperecomputing dot com]

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

--- Comment #4 from Hao Liu <hliu at amperecomputing dot com> ---
> IMHO, the initialization with false is unnecessary and very likely it isn't able to get optimized, it seems worse from this point of view.

Sorry. I don't think so. See more at
https://www.oreilly.com/library/view/c-coding-standards/0321113586/ch20.html:

Start with a clean slate: Uninitialized variables are a common source of bugs
in C and C++ programs. There are few reasons to ever leave a variable
uninitialized. None is serious enough to justify the hazard of undefined
behavior.

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[hliu at amperecomputing dot com]

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

--- Comment #5 from Hao Liu <hliu at amperecomputing dot com> ---
BTW, there is no warning is probably because the original code is too
complicated and not inlined. 
Compile the simple case by "g++ -O3 -S -Wall hello.c":
int foo(bool a) {
  bool b;
  if (a || b)
    return 1;
  b = true;
  return 0;
}

gcc report warning:
hello.c: In function ‘int foo(bool)’:
hello.c:4:9: warning: ‘b’ is used uninitialized [-Wuninitialized]
    4 |   if (a || b)
      |       ~~^~~~

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[linkw at gcc dot gnu.org]

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

--- Comment #6 from Kewen Lin <linkw at gcc dot gnu.org> ---
It's an arguable topic, I can't find the thread that previously some reviewers
told me it's not always good to initialize the local variable. IIRC, the case
is that I initialized one variable at the top, but the one got assigned in
below all switch cases, I was told not to initialize it, once a new case arm
use that variable unexpectedly, some bug would show up (static analyzer or
runtime bug), a default (but not reasonable) value is easy to cover some issue,
though at that time I hold an argument that initializing local variable seems
best practice.

For this case, since vect_analyze_loop_2 isn't able to be inlined, the
initialization can't be optimized, the worse thing I meant is to have more
instructions than before, for example, a test case like:

__attribute__((noipa)) int foo(int *a) { *a = 1; return 1;}

int test(){
#ifdef AINIT
  int a = 0;
#else
  int a;
#endif
  int b = foo(&a);
  return b;
}

on Power, I got one more li and stw for -DAINIT.

But I just posted my naive thoughts, if you still think it's better to
initialize it, you can just go ahead to post a patch. :)

----

btw, the test case

int foo() {
  bool a = true;
  bool b;
  if (a || b)
    return 1;
  b = true;
  return 0;
}

still has the warning, it looks something can be improved (guess we prefer not
to emit warning).

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[hliu at amperecomputing dot com]

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

--- Comment #7 from Hao Liu <hliu at amperecomputing dot com> ---
> int foo() {
>   bool a = true;
>   bool b;
>   if (a || b)
>     return 1;
>   b = true;
>   return 0;
> }
> 
> still has the warning, it looks something can be improved (guess we prefer not to emit warning).

Your case is wrong, you should initialize "b" and there will be no warning.


> __attribute__((noipa)) int foo(int *a) { *a = 1; return 1;}
> 
> int test(){
> #ifdef AINIT
>   int a = 0;
> #else
>   int a;
> #endif
>   int b = foo(&a);
>   return b;
> }

This case doesn't have problem. If "foo" uses "a" directly, the result is
undefined behavior, which causes both correctness and performance issues.

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[linkw at gcc dot gnu.org]

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

--- Comment #8 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Hao Liu from comment #7)
> > int foo() {
> >   bool a = true;
> >   bool b;
> >   if (a || b)
> >     return 1;
> >   b = true;
> >   return 0;
> > }
> > 
> > still has the warning, it looks something can be improved (guess we prefer not to emit warning).
> 
> Your case is wrong, you should initialize "b" and there will be no warning.
> 

I don't follow why it is wrong, doesn't short-circuiting make b uninitialized
trivial?

> 
> > __attribute__((noipa)) int foo(int *a) { *a = 1; return 1;}
> > 
> > int test(){
> > #ifdef AINIT
> >   int a = 0;
> > #else
> >   int a;
> > #endif
> >   int b = foo(&a);
> >   return b;
> > }
> 
> This case doesn't have problem. If "foo" uses "a" directly, the result is
> undefined behavior, which causes both correctness and performance issues.

foo is just an example for not getting inlined, the point here is extra cost
paid.

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Hao Liu <hliu@gcc.gnu.org>:

https://gcc.gnu.org/g:2c12ccf800fc7890925402d30a02f0fa9e2627cc

commit r14-2290-g2c12ccf800fc7890925402d30a02f0fa9e2627cc
Author: Hao Liu <hliu@os.amperecomputing.com>
Date:   Tue Jul 4 17:17:50 2023 +0800

    PR tree-optimization/110531 - Vect: avoid using uninitialized variable

    slp_done_for_suggested_uf is used directly in vect_analyze_loop_2
    without initialization, which is undefined behavior.  Initialize it to
false
    according to the discussion.

    gcc/ChangeLog:
            PR tree-optimization/110531
            * tree-vect-loop.cc (vect_analyze_loop_1): initialize
            slp_done_for_suggested_uf to false.

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[hliu at amperecomputing dot com]

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

--- Comment #10 from Hao Liu <hliu at amperecomputing dot com> ---
> foo is just an example for not getting inlined, the point here is extra cost paid.

My point is that the case is different from the original case in
tree-vect-loop.cc.  For example, change the case as following:

__attribute__((noipa)) int foo(int *a) { *a == 1 ? return 1 : return 0;}

That's similar to the original problem (the value of "a" is undefiend).

I don't mean that "a" must be initialized in test().  We can also initalize "a"
in foo, but should not use "a" before initialization. E.g.

__attribute__((noipa)) int foo(int *a) { 
  *a == 1;
  ...
  if (*a)
}

The above case has no problem.

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[linkw at gcc dot gnu.org]

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

--- Comment #11 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Hao Liu from comment #10)
> > foo is just an example for not getting inlined, the point here is extra cost paid.
> 
> My point is that the case is different from the original case in
> tree-vect-loop.cc.  For example, change the case as following:
> 
> __attribute__((noipa)) int foo(int *a) { *a == 1 ? return 1 : return 0;}
> 
> That's similar to the original problem (the value of "a" is undefiend).
> 
> I don't mean that "a" must be initialized in test().  We can also initalize
> "a" in foo, but should not use "a" before initialization. E.g.
> 
> __attribute__((noipa)) int foo(int *a) { 
>   *a == 1;
>   ...
>   if (*a)
> }
> 
> The above case has no problem.

Yeah, I got your point here, but my case above is mainly to show that we need
to pay a bit more cost to initialize variable (there foo is just a random one
which isn't inlined). Anyway, thanks for further clarifying.

[Bug tree-optimization/110531] Vect: slp_done_for_suggested_uf is not initialized in tree-vect-loop.cc

[hliu at amperecomputing dot com]

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

Hao Liu <hliu at amperecomputing dot com> changed:

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

--- Comment #12 from Hao Liu <hliu at amperecomputing dot com> ---
OK. Now I got your point of useless initialization may introduce extra cost.