[Bug middle-end/61409] [5/6/7 regression] -Wmaybe-uninitialized false-positive with -O2

[aldyh at gcc dot gnu.org]

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

--- Comment #25 from Aldy Hernandez  ---
Author: aldyh
Date: Sun Nov 20 18:34:06 2016
New Revision: 242639

URL: https://gcc.gnu.org/viewcvs?rev=242639=gcc=rev
Log:
PR middle-end/61409
* tree-ssa-uninit.c: Define new global max_phi_args.
(compute_uninit_opnds_pos): Use max_phi_args.
(prune_uninit_phi_opnds): Same.
(use_pred_not_overlap_with_undef_path_pred): Remove reference to
missing NUM_PREDS in function comment.
(can_one_predicate_be_invalidated_p): New.
(can_chain_union_be_invalidated_p): New.
(flatten_out_predicate_chains): New.
(uninit_ops_invalidate_phi_use): New.
(is_use_properly_guarded): Call uninit_ops_invalidate_phi_use.

Added:
trunk/gcc/testsuite/gcc.dg/uninit-pr61409.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/tree-ssa-uninit.c

[Bug middle-end/61409] [5/6/7 regression] -Wmaybe-uninitialized false-positive with -O2

[law at redhat dot com]

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

--- Comment #24 from Jeffrey A. Law  ---
OK.  It's coming back to me now.  And yes, Aldy, it was the edge 9->6 :-)

So we have a PHI argument that references an uninitialized variable.  There is
a control predicate for that PHI argument, call it p.

The use of the result of the PHI is also guarded.  In this particular case the
guard is !p.

Thus there is no path through the CFG which uses the uninitialized variable. 
We ought to be able to look at the guard of the PHI argument as well as the
guard for the use, at least that's the theory.  Now onward to look at your
patch...

[Bug middle-end/61409] [5/6/7 regression] -Wmaybe-uninitialized false-positive with -O2

[law at redhat dot com]

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

--- Comment #23 from Jeffrey A. Law  ---
Sorry, I can't remember if I meant 9->5 or 9->6 at this point :-)  I need to
refamiliarize myself with this stuff again to make sure I've got the basic
concepts before reviewing the patch.

But you can probably see now why I said there was some seriously powerful, but
dense code in here that I'd like to be able to re-use elsewhere to prune other
false positive may-be warnings.

[Bug middle-end/61409] [5/6/7 regression] -Wmaybe-uninitialized false-positive with -O2

[aldyh at gcc dot gnu.org]

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

--- Comment #22 from Aldy Hernandez  ---
Created attachment 39499
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=39499=edit
untested patch v1

I think I see what Jeff is getting at.

Here is an untested patch exploring the idea of ignoring unguarded uses if we
can prove that the guards for such uses are invalidated by the uninitialized
operand paths being executed.

Preliminary tests show that it fixes the testcase in the PR without introducing
any regressions in the rest of the uninit tests:

 make check-gcc RUNTESTFLAGS=dg.exp=uninit*

As the "NOTE:" in the code states, we could be much smarter when invalidating
predicates, but for now let's do straight negation which works for the simple
case.

Does this seem like a reasonable approach?

[Bug middle-end/61409] [5/6/7 regression] -Wmaybe-uninitialized false-positive with -O2

[aldyh at gcc dot gnu.org]

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

--- Comment #20 from Aldy Hernandez  ---
If anyone is interested, here is an even smaller testcase:

int *rw;
int line_height;
int pixel_width;
int text_cols;
int width1, width2, width3;

void *pointer;

void f (int i, int j)
{
  void *ptr;
  if (i)
{
  if (j)
return;
  ptr = pointer;
}
  pixel_width = 1234 + width1 + 2 * width2 + 2 * width3;
  *rw = text_cols + line_height;
  if (i)
rw=ptr;
}

[Bug middle-end/61409] [5/6/7 regression] -Wmaybe-uninitialized false-positive with -O2

[aldyh at gcc dot gnu.org]

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

--- Comment #21 from Aldy Hernandez  ---
Created attachment 39487
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=39487=edit
even more^2 reduced testcase

Smaller testcase without any structure nonsense and even less variables.

[Bug middle-end/61409] [5/6/7 regression] -Wmaybe-uninitialized false-positive with -O2

[aldyh at gcc dot gnu.org]

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

--- Comment #19 from Aldy Hernandez  ---
Also, unless I'm missing something, in Jeff's analysis, I see no reference to
j, which plays a pivotal role.

In the testcase in comment 14, we can see that the guard for ptr_14 is actually
[i && j==0]:

if (i)
  {
if (j)
  return; /* bar(); */
ptr = init();
  }

...because on j != 0, we exit, which is what the .uninit dump is suggesting
with:

[AFTER NORMALIZATION -- [DEF]:
ptr_14 = PHI 
is guarded by :

 (.NOT.) j_20(D) != 0 (.AND.) i_17(D) != 0

Meanwhile, the use we are warning on is [i != 0]:

[AFTER NORMALIZATION -- [USE]:
rw = ptr_14;
is guarded by :

i_17(D) != 0

So the problem is that while the guard for the DEF is [i != 0 && j == 0], the
guard for the USE is only [i != 0].  Uninit should somehow be aware that if PTR
was set, then  j==0, because otherwise we would've exited the function.

You can see that .uninit is expecting the use to be guarded by [i != 0 && j ==
0], by adding "&& !j" to the guard and silencing the warning:

   if (i && !j)
{
  rw=ptr;
}

[Bug middle-end/61409] [5/6/7 regression] -Wmaybe-uninitialized false-positive with -O2

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez  changed:

   What|Removed |Added

 CC||aldyh at gcc dot gnu.org

--- Comment #18 from Aldy Hernandez  ---
(In reply to Jeffrey A. Law from comment #16)
> AFAICT tree-ssa-uninit won't look at the predicate associated with an
> undefined PHI argument and test it against the predicate for the actual use.
> 
> ie, given this PHI from the testcase:
> 
> ;;   basic block 6, loop depth 0
> ;;pred:   9
> ;;5
>   # ptr_1 = PHI 
> 
> We want to look at the control dependent path that leads to the edge (9,5). 
> For this test, that edge is control dependent on bb2:

Jeff, do you mean edge(9,6) throughout?  Because I don't see any flow of
control from 9->5.