Re: A bug in vrp_meet?

[Qing Zhao <qing.zhao@oracle.com>]

Hi, Richard,

> On Mar 4, 2019, at 5:45 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>> 
>> It looks like DOM fails to visit stmts generated by simplification. Can you open a bug report with a testcase?
>> 
>> 
>> The problem is, It took me quite some time in order to come up with a small and independent testcase for this problem,
>> a little bit change made the error disappear.
>> 
>> do you have any suggestion on this?  or can you give me some hint on how to fix this in DOM?  then I can try the fix on my side?
> 
> I remember running into similar issues in the past where I tried to
> extract temporary nonnull ranges from divisions.
> I have there
> 
> @@ -1436,11 +1436,16 @@ dom_opt_dom_walker::before_dom_children
>   m_avail_exprs_stack->pop_to_marker ();
> 
>   edge taken_edge = NULL;
> -  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> -    {
> -      evrp_range_analyzer.record_ranges_from_stmt (gsi_stmt (gsi), false);
> -      taken_edge = this->optimize_stmt (bb, gsi);
> -    }
> +  gsi = gsi_start_bb (bb);
> +  if (!gsi_end_p (gsi))
> +    while (1)
> +      {
> +       evrp_range_analyzer.record_def_ranges_from_stmt (gsi_stmt (gsi), false);
> +       taken_edge = this->optimize_stmt (bb, &gsi);
> +       if (gsi_end_p (gsi))
> +         break;
> +       evrp_range_analyzer.record_use_ranges_from_stmt (gsi_stmt (gsi));
> +      }
> 
>   /* Now prepare to process dominated blocks.  */
>   record_edge_info (bb);
> 
> OTOH the issue in your case is that fold emits new stmts before gsi but the
> above loop will never look at them.  See tree-ssa-forwprop.c for code how
> to deal with this (setting a pass-local flag on stmts visited and walking back
> to unvisited, newly inserted ones).  The fold_stmt interface could in theory
> also be extended to insert new stmts on a sequence passed to it so the
> caller would be responsible for inserting them into the IL and could then
> more easily revisit them (but that's a bigger task).
> 
> So, does the following help?

Yes, this change fixed the error in my side, now, in the dumped file for pass dom3:

====
Visiting statement:
i_49 = _98 > 0 ? k_105 : 0;
Meeting
  [0, 65535]
and
  [0, 0]
to
  [0, 65535]
Intersecting
  [0, 65535]
and
  [0, 65535]
to
  [0, 65535]
Optimizing statement i_49 = _98 > 0 ? k_105 : 0;
  Replaced 'k_105' with variable '_98'
gimple_simplified to _152 = MAX_EXPR <_98, 0>;
i_49 = _152;
  Folded to: i_49 = _152;
LKUP STMT i_49 = _152
==== ASGN i_49 = _152

Visiting statement:
_152 = MAX_EXPR <_98, 0>;

Visiting statement:
i_49 = _152;
Intersecting
  [0, 65535]  EQUIVALENCES: { _152 } (1 elements)
and
  [0, 65535]
to
  [0, 65535]  EQUIVALENCES: { _152 } (1 elements)
====

We can clearly see from the above, all the new stmts generated by fold are visited now. 

it is also confirmed that the runtime error caused by this bug was gone with this fix.

So, what’s the next step for this issue?

will you commit this fix to gcc9 and gcc8  (we need it in gcc8)?

or I can test this fix on my side and commit it to both gcc9 and gcc8?

thanks.

Qing

> 
> Index: gcc/tree-ssa-dom.c
> ===================================================================
> --- gcc/tree-ssa-dom.c  (revision 269361)
> +++ gcc/tree-ssa-dom.c  (working copy)
> @@ -1482,8 +1482,25 @@ dom_opt_dom_walker::before_dom_children
>   edge taken_edge = NULL;
>   for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>     {
> +      gimple_stmt_iterator pgsi = gsi;
> +      gsi_prev (&pgsi);
>       evrp_range_analyzer.record_ranges_from_stmt (gsi_stmt (gsi), false);
>       taken_edge = this->optimize_stmt (bb, gsi);
> +      gimple_stmt_iterator npgsi = gsi;
> +      gsi_prev (&npgsi);
> +      /* Walk new stmts eventually inserted by DOM.  gsi_stmt (gsi) itself
> +        while it may be changed should not have gotten a new definition.  */
> +      if (gsi_stmt (pgsi) != gsi_stmt (npgsi))
> +       do
> +         {
> +           if (gsi_end_p (pgsi))
> +             pgsi = gsi_start_bb (bb);
> +           else
> +             gsi_next (&pgsi);
> +           evrp_range_analyzer.record_ranges_from_stmt (gsi_stmt (pgsi),
> +                                                        false);
> +         }
> +       while (gsi_stmt (pgsi) != gsi_stmt (gsi));
>     }
> 
>   /* Now prepare to process dominated blocks.  */
> 
> 
> Richard.
> 
>> Thanks a lot.
>> 
>> Qing
>> 
>> 
>> 
>> Richard.
>> 
>>