Re: A bug in vrp_meet?

[Richard Biener]

On Sun, May 5, 2019 at 11:09 PM Eric Botcazou  wrote:
>
> > I have now applied this variant.
>
> You backported it onto the 8 branch on Friday:
>
> 2019-05-03  Richard Biener  
>
> Backport from mainline
> [...]
> 2019-03-07  Richard Biener  
>
> PR tree-optimization/89595
> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> stmt iterator as reference, take boolean output parameter to
> indicate whether the stmt was removed and thus the iterator
> already advanced.
> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> stmts created by folding.
>
> and this introduced a regression for the attached Ada testcase at -O:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x0102173c in set_value_range (
> vr=0x17747a0  const*)::vr_const_varying>, t=VR_RANGE, min=0x76c3df78, max= out>, equiv=0x0)
> at /home/eric/svn/gcc-8-branch/gcc/tree-vrp.c:298
> 298   vr->type = t;
>
> on x86-64 at least.  Mainline and 9 branch are not affected.

It looks like backporting r269597 might fix it though reverting that on trunk
doesn't make the testcase fail there.

Richard.

> --
> Eric Botcazou

Re: A bug in vrp_meet?

[Richard Biener]

On Sun, May 5, 2019 at 11:09 PM Eric Botcazou  wrote:
>
> > I have now applied this variant.
>
> You backported it onto the 8 branch on Friday:
>
> 2019-05-03  Richard Biener  
>
> Backport from mainline
> [...]
> 2019-03-07  Richard Biener  
>
> PR tree-optimization/89595
> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> stmt iterator as reference, take boolean output parameter to
> indicate whether the stmt was removed and thus the iterator
> already advanced.
> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> stmts created by folding.
>
> and this introduced a regression for the attached Ada testcase at -O:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x0102173c in set_value_range (
> vr=0x17747a0  const*)::vr_const_varying>, t=VR_RANGE, min=0x76c3df78, max= out>, equiv=0x0)
> at /home/eric/svn/gcc-8-branch/gcc/tree-vrp.c:298
> 298   vr->type = t;
>
> on x86-64 at least.  Mainline and 9 branch are not affected.

It looks like backporting r269597 might fix it though reverting that on trunk
doesn't make the testcase fail there.

Richard.

> --
> Eric Botcazou

Re: A bug in vrp_meet?

[Eric Botcazou]

> I have now applied this variant.

You backported it onto the 8 branch on Friday:

2019-05-03  Richard Biener  

Backport from mainline
[...]
2019-03-07  Richard Biener  

PR tree-optimization/89595
* tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
stmt iterator as reference, take boolean output parameter to
indicate whether the stmt was removed and thus the iterator
already advanced.
(dom_opt_dom_walker::before_dom_children): Re-iterate over
stmts created by folding.

and this introduced a regression for the attached Ada testcase at -O:

Program received signal SIGSEGV, Segmentation fault.
0x0102173c in set_value_range (
vr=0x17747a0 , t=VR_RANGE, min=0x76c3df78, max=, equiv=0x0)
at /home/eric/svn/gcc-8-branch/gcc/tree-vrp.c:298
298   vr->type = t;

on x86-64 at least.  Mainline and 9 branch are not affected.

-- 
Eric Botcazoupackage Opt78 is

   subtype Reasonable is Integer range 1..10;

   type UC (D: Reasonable := 2) is record
  S: String (1 .. D) := "Hi";
   end record;

   type AUC is access all UC;

   procedure Proc (P : UC; Msg : String);

end Opt78;
-- { dg-do compile }
-- { dg-options "-O" }

package body Opt78 is

   procedure Proc (P : UC; Msg : String) is
  Default : UC := (1, "!");
   begin
  if P = Default then
 raise Program_Error;
  else
 raise Constraint_Error;
  end if;
   end;

end Opt78;

Re: A bug in vrp_meet?

[Eric Botcazou]

> I have now applied this variant.

You backported it onto the 8 branch on Friday:

2019-05-03  Richard Biener  

Backport from mainline
[...]
2019-03-07  Richard Biener  

PR tree-optimization/89595
* tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
stmt iterator as reference, take boolean output parameter to
indicate whether the stmt was removed and thus the iterator
already advanced.
(dom_opt_dom_walker::before_dom_children): Re-iterate over
stmts created by folding.

and this introduced a regression for the attached Ada testcase at -O:

Program received signal SIGSEGV, Segmentation fault.
0x0102173c in set_value_range (
vr=0x17747a0 , t=VR_RANGE, min=0x76c3df78, max=, equiv=0x0)
at /home/eric/svn/gcc-8-branch/gcc/tree-vrp.c:298
298   vr->type = t;

on x86-64 at least.  Mainline and 9 branch are not affected.

-- 
Eric Botcazoupackage Opt78 is

   subtype Reasonable is Integer range 1..10;

   type UC (D: Reasonable := 2) is record
  S: String (1 .. D) := "Hi";
   end record;

   type AUC is access all UC;

   procedure Proc (P : UC; Msg : String);

end Opt78;
-- { dg-do compile }
-- { dg-options "-O" }

package body Opt78 is

   procedure Proc (P : UC; Msg : String) is
  Default : UC := (1, "!");
   begin
  if P = Default then
 raise Program_Error;
  else
 raise Constraint_Error;
  end if;
   end;

end Opt78;

Re: A bug in vrp_meet?

[Richard Biener]

On Tue, Mar 19, 2019 at 8:53 PM Jeff Law  wrote:
>
> On 3/6/19 3:05 AM, Richard Biener wrote:
> > On Tue, Mar 5, 2019 at 10:36 PM Jeff Law  wrote:
> >>
> >> On 3/5/19 7:44 AM, Richard Biener wrote:
> >>
> >>> So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> >>> the MAX_EXPR introduced by folding makes it somewhat ugly.
> >>>
> >>> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> >>>
> >>> Any ideas how to make it less so?  I can split out making optimize_stmt
> >>> take a gsi * btw, in case that's a more obvious change and it makes the
> >>> patch a little smaller.
> >>>
> >>> Richard.
> >>>
> >>> 2019-03-05  Richard Biener  
> >>>
> >>> PR tree-optimization/89595
> >>> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> >>> stmt iterator as reference, take boolean output parameter to
> >>> indicate whether the stmt was removed and thus the iterator
> >>> already advanced.
> >>> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> >>> stmts created by folding.
> >>>
> >>> * gcc.dg/torture/pr89595.c: New testcase.
> >>>
> >>
> >> Well, all the real logic changs are in the before_dom_children method.
> >> The bits in optimize_stmt are trivial enough to effectively ignore.
> >>
> >> I don't see a better way to discover and process statements that are
> >> created in the bowels of fold_stmt.
> >
> > I'm not entirely happy so I created the following alternative which
> > is a bit larger and slower due to the pre-pass clearing the visited flag
> > but is IMHO easier to follow.  I guess there's plenty of TLC opportunity
> > here but then I also hope to retire the VN parts of DOM in favor
> > of the non-iterating RPO-VN code...
> >
> > So - I'd lean to this variant even though it has the extra loop over stmts,
> > would you agree?
> >
> > Bootstrap / regtest running on x86_64-unknown-linux-gnu.
> >
> > Richard.
> >
> > 2019-03-06  Richard Biener  
> >
> > PR tree-optimization/89595
> > * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> > stmt iterator as reference, take boolean output parameter to
> > indicate whether the stmt was removed and thus the iterator
> > already advanced.
> > (dom_opt_dom_walker::before_dom_children): Re-iterate over
> > stmts created by folding.
> >
> > * gcc.dg/torture/pr89595.c: New testcase.
> This one is easier to follow from a logic standpoint.  I don't think the
> gimple_set_visited bits are going to be terribly expensive in general.
>
> Is that flag in a known state for new statements?  I'm guessing it's
> cleared by some structure-sized memset as we create the raw statement?

Yes, it's of course not documented that way but IMHo the only reasonable
state.

> Might be worth clarifying that in the comments in gimple.h.
>
> jeff
>

Re: A bug in vrp_meet?

[Richard Biener]

On Tue, Mar 19, 2019 at 8:53 PM Jeff Law  wrote:
>
> On 3/6/19 3:05 AM, Richard Biener wrote:
> > On Tue, Mar 5, 2019 at 10:36 PM Jeff Law  wrote:
> >>
> >> On 3/5/19 7:44 AM, Richard Biener wrote:
> >>
> >>> So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> >>> the MAX_EXPR introduced by folding makes it somewhat ugly.
> >>>
> >>> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> >>>
> >>> Any ideas how to make it less so?  I can split out making optimize_stmt
> >>> take a gsi * btw, in case that's a more obvious change and it makes the
> >>> patch a little smaller.
> >>>
> >>> Richard.
> >>>
> >>> 2019-03-05  Richard Biener  
> >>>
> >>> PR tree-optimization/89595
> >>> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> >>> stmt iterator as reference, take boolean output parameter to
> >>> indicate whether the stmt was removed and thus the iterator
> >>> already advanced.
> >>> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> >>> stmts created by folding.
> >>>
> >>> * gcc.dg/torture/pr89595.c: New testcase.
> >>>
> >>
> >> Well, all the real logic changs are in the before_dom_children method.
> >> The bits in optimize_stmt are trivial enough to effectively ignore.
> >>
> >> I don't see a better way to discover and process statements that are
> >> created in the bowels of fold_stmt.
> >
> > I'm not entirely happy so I created the following alternative which
> > is a bit larger and slower due to the pre-pass clearing the visited flag
> > but is IMHO easier to follow.  I guess there's plenty of TLC opportunity
> > here but then I also hope to retire the VN parts of DOM in favor
> > of the non-iterating RPO-VN code...
> >
> > So - I'd lean to this variant even though it has the extra loop over stmts,
> > would you agree?
> >
> > Bootstrap / regtest running on x86_64-unknown-linux-gnu.
> >
> > Richard.
> >
> > 2019-03-06  Richard Biener  
> >
> > PR tree-optimization/89595
> > * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> > stmt iterator as reference, take boolean output parameter to
> > indicate whether the stmt was removed and thus the iterator
> > already advanced.
> > (dom_opt_dom_walker::before_dom_children): Re-iterate over
> > stmts created by folding.
> >
> > * gcc.dg/torture/pr89595.c: New testcase.
> This one is easier to follow from a logic standpoint.  I don't think the
> gimple_set_visited bits are going to be terribly expensive in general.
>
> Is that flag in a known state for new statements?  I'm guessing it's
> cleared by some structure-sized memset as we create the raw statement?

Yes, it's of course not documented that way but IMHo the only reasonable
state.

> Might be worth clarifying that in the comments in gimple.h.
>
> jeff
>

Re: A bug in vrp_meet?

[Jeff Law]

On 3/6/19 3:05 AM, Richard Biener wrote:
> On Tue, Mar 5, 2019 at 10:36 PM Jeff Law  wrote:
>>
>> On 3/5/19 7:44 AM, Richard Biener wrote:
>>
>>> So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
>>> the MAX_EXPR introduced by folding makes it somewhat ugly.
>>>
>>> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>>>
>>> Any ideas how to make it less so?  I can split out making optimize_stmt
>>> take a gsi * btw, in case that's a more obvious change and it makes the
>>> patch a little smaller.
>>>
>>> Richard.
>>>
>>> 2019-03-05  Richard Biener  
>>>
>>> PR tree-optimization/89595
>>> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
>>> stmt iterator as reference, take boolean output parameter to
>>> indicate whether the stmt was removed and thus the iterator
>>> already advanced.
>>> (dom_opt_dom_walker::before_dom_children): Re-iterate over
>>> stmts created by folding.
>>>
>>> * gcc.dg/torture/pr89595.c: New testcase.
>>>
>>
>> Well, all the real logic changs are in the before_dom_children method.
>> The bits in optimize_stmt are trivial enough to effectively ignore.
>>
>> I don't see a better way to discover and process statements that are
>> created in the bowels of fold_stmt.
> 
> I'm not entirely happy so I created the following alternative which
> is a bit larger and slower due to the pre-pass clearing the visited flag
> but is IMHO easier to follow.  I guess there's plenty of TLC opportunity
> here but then I also hope to retire the VN parts of DOM in favor
> of the non-iterating RPO-VN code...
> 
> So - I'd lean to this variant even though it has the extra loop over stmts,
> would you agree?
> 
> Bootstrap / regtest running on x86_64-unknown-linux-gnu.
> 
> Richard.
> 
> 2019-03-06  Richard Biener  
> 
> PR tree-optimization/89595
> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> stmt iterator as reference, take boolean output parameter to
> indicate whether the stmt was removed and thus the iterator
> already advanced.
> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> stmts created by folding.
> 
> * gcc.dg/torture/pr89595.c: New testcase.
This one is easier to follow from a logic standpoint.  I don't think the
gimple_set_visited bits are going to be terribly expensive in general.

Is that flag in a known state for new statements?  I'm guessing it's
cleared by some structure-sized memset as we create the raw statement?

Might be worth clarifying that in the comments in gimple.h.

jeff

Re: A bug in vrp_meet?

[Jeff Law]

On 3/6/19 3:05 AM, Richard Biener wrote:
> On Tue, Mar 5, 2019 at 10:36 PM Jeff Law  wrote:
>>
>> On 3/5/19 7:44 AM, Richard Biener wrote:
>>
>>> So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
>>> the MAX_EXPR introduced by folding makes it somewhat ugly.
>>>
>>> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>>>
>>> Any ideas how to make it less so?  I can split out making optimize_stmt
>>> take a gsi * btw, in case that's a more obvious change and it makes the
>>> patch a little smaller.
>>>
>>> Richard.
>>>
>>> 2019-03-05  Richard Biener  
>>>
>>> PR tree-optimization/89595
>>> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
>>> stmt iterator as reference, take boolean output parameter to
>>> indicate whether the stmt was removed and thus the iterator
>>> already advanced.
>>> (dom_opt_dom_walker::before_dom_children): Re-iterate over
>>> stmts created by folding.
>>>
>>> * gcc.dg/torture/pr89595.c: New testcase.
>>>
>>
>> Well, all the real logic changs are in the before_dom_children method.
>> The bits in optimize_stmt are trivial enough to effectively ignore.
>>
>> I don't see a better way to discover and process statements that are
>> created in the bowels of fold_stmt.
> 
> I'm not entirely happy so I created the following alternative which
> is a bit larger and slower due to the pre-pass clearing the visited flag
> but is IMHO easier to follow.  I guess there's plenty of TLC opportunity
> here but then I also hope to retire the VN parts of DOM in favor
> of the non-iterating RPO-VN code...
> 
> So - I'd lean to this variant even though it has the extra loop over stmts,
> would you agree?
> 
> Bootstrap / regtest running on x86_64-unknown-linux-gnu.
> 
> Richard.
> 
> 2019-03-06  Richard Biener  
> 
> PR tree-optimization/89595
> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> stmt iterator as reference, take boolean output parameter to
> indicate whether the stmt was removed and thus the iterator
> already advanced.
> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> stmts created by folding.
> 
> * gcc.dg/torture/pr89595.c: New testcase.
This one is easier to follow from a logic standpoint.  I don't think the
gimple_set_visited bits are going to be terribly expensive in general.

Is that flag in a known state for new statements?  I'm guessing it's
cleared by some structure-sized memset as we create the raw statement?

Might be worth clarifying that in the comments in gimple.h.

jeff

Re: A bug in vrp_meet?

[Richard Biener]

On Wed, Mar 6, 2019 at 11:05 AM Richard Biener
 wrote:
>
> On Tue, Mar 5, 2019 at 10:36 PM Jeff Law  wrote:
> >
> > On 3/5/19 7:44 AM, Richard Biener wrote:
> >
> > > So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> > > the MAX_EXPR introduced by folding makes it somewhat ugly.
> > >
> > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> > >
> > > Any ideas how to make it less so?  I can split out making optimize_stmt
> > > take a gsi * btw, in case that's a more obvious change and it makes the
> > > patch a little smaller.
> > >
> > > Richard.
> > >
> > > 2019-03-05  Richard Biener  
> > >
> > > PR tree-optimization/89595
> > > * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> > > stmt iterator as reference, take boolean output parameter to
> > > indicate whether the stmt was removed and thus the iterator
> > > already advanced.
> > > (dom_opt_dom_walker::before_dom_children): Re-iterate over
> > > stmts created by folding.
> > >
> > > * gcc.dg/torture/pr89595.c: New testcase.
> > >
> >
> > Well, all the real logic changs are in the before_dom_children method.
> > The bits in optimize_stmt are trivial enough to effectively ignore.
> >
> > I don't see a better way to discover and process statements that are
> > created in the bowels of fold_stmt.
>
> I'm not entirely happy so I created the following alternative which
> is a bit larger and slower due to the pre-pass clearing the visited flag
> but is IMHO easier to follow.  I guess there's plenty of TLC opportunity
> here but then I also hope to retire the VN parts of DOM in favor
> of the non-iterating RPO-VN code...
>
> So - I'd lean to this variant even though it has the extra loop over stmts,
> would you agree?

I have now applied this variant.

Richard.

> Bootstrap / regtest running on x86_64-unknown-linux-gnu.
>
> Richard.
>
> 2019-03-06  Richard Biener  
>
> PR tree-optimization/89595
> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> stmt iterator as reference, take boolean output parameter to
> indicate whether the stmt was removed and thus the iterator
> already advanced.
> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> stmts created by folding.
>
> * gcc.dg/torture/pr89595.c: New testcase.

Re: A bug in vrp_meet?

[Richard Biener]

On Wed, Mar 6, 2019 at 11:05 AM Richard Biener
 wrote:
>
> On Tue, Mar 5, 2019 at 10:36 PM Jeff Law  wrote:
> >
> > On 3/5/19 7:44 AM, Richard Biener wrote:
> >
> > > So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> > > the MAX_EXPR introduced by folding makes it somewhat ugly.
> > >
> > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> > >
> > > Any ideas how to make it less so?  I can split out making optimize_stmt
> > > take a gsi * btw, in case that's a more obvious change and it makes the
> > > patch a little smaller.
> > >
> > > Richard.
> > >
> > > 2019-03-05  Richard Biener  
> > >
> > > PR tree-optimization/89595
> > > * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> > > stmt iterator as reference, take boolean output parameter to
> > > indicate whether the stmt was removed and thus the iterator
> > > already advanced.
> > > (dom_opt_dom_walker::before_dom_children): Re-iterate over
> > > stmts created by folding.
> > >
> > > * gcc.dg/torture/pr89595.c: New testcase.
> > >
> >
> > Well, all the real logic changs are in the before_dom_children method.
> > The bits in optimize_stmt are trivial enough to effectively ignore.
> >
> > I don't see a better way to discover and process statements that are
> > created in the bowels of fold_stmt.
>
> I'm not entirely happy so I created the following alternative which
> is a bit larger and slower due to the pre-pass clearing the visited flag
> but is IMHO easier to follow.  I guess there's plenty of TLC opportunity
> here but then I also hope to retire the VN parts of DOM in favor
> of the non-iterating RPO-VN code...
>
> So - I'd lean to this variant even though it has the extra loop over stmts,
> would you agree?

I have now applied this variant.

Richard.

> Bootstrap / regtest running on x86_64-unknown-linux-gnu.
>
> Richard.
>
> 2019-03-06  Richard Biener  
>
> PR tree-optimization/89595
> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> stmt iterator as reference, take boolean output parameter to
> indicate whether the stmt was removed and thus the iterator
> already advanced.
> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> stmts created by folding.
>
> * gcc.dg/torture/pr89595.c: New testcase.

Re: A bug in vrp_meet?

[Richard Biener]

On Tue, Mar 5, 2019 at 10:36 PM Jeff Law  wrote:
>
> On 3/5/19 7:44 AM, Richard Biener wrote:
>
> > So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> > the MAX_EXPR introduced by folding makes it somewhat ugly.
> >
> > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> >
> > Any ideas how to make it less so?  I can split out making optimize_stmt
> > take a gsi * btw, in case that's a more obvious change and it makes the
> > patch a little smaller.
> >
> > Richard.
> >
> > 2019-03-05  Richard Biener  
> >
> > PR tree-optimization/89595
> > * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> > stmt iterator as reference, take boolean output parameter to
> > indicate whether the stmt was removed and thus the iterator
> > already advanced.
> > (dom_opt_dom_walker::before_dom_children): Re-iterate over
> > stmts created by folding.
> >
> > * gcc.dg/torture/pr89595.c: New testcase.
> >
>
> Well, all the real logic changs are in the before_dom_children method.
> The bits in optimize_stmt are trivial enough to effectively ignore.
>
> I don't see a better way to discover and process statements that are
> created in the bowels of fold_stmt.

I'm not entirely happy so I created the following alternative which
is a bit larger and slower due to the pre-pass clearing the visited flag
but is IMHO easier to follow.  I guess there's plenty of TLC opportunity
here but then I also hope to retire the VN parts of DOM in favor
of the non-iterating RPO-VN code...

So - I'd lean to this variant even though it has the extra loop over stmts,
would you agree?

Bootstrap / regtest running on x86_64-unknown-linux-gnu.

Richard.

2019-03-06  Richard Biener  

PR tree-optimization/89595
* tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
stmt iterator as reference, take boolean output parameter to
indicate whether the stmt was removed and thus the iterator
already advanced.
(dom_opt_dom_walker::before_dom_children): Re-iterate over
stmts created by folding.

* gcc.dg/torture/pr89595.c: New testcase.


fix-pr89595-2
Description: Binary data

Re: A bug in vrp_meet?

[Richard Biener]

On Tue, Mar 5, 2019 at 10:36 PM Jeff Law  wrote:
>
> On 3/5/19 7:44 AM, Richard Biener wrote:
>
> > So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> > the MAX_EXPR introduced by folding makes it somewhat ugly.
> >
> > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> >
> > Any ideas how to make it less so?  I can split out making optimize_stmt
> > take a gsi * btw, in case that's a more obvious change and it makes the
> > patch a little smaller.
> >
> > Richard.
> >
> > 2019-03-05  Richard Biener  
> >
> > PR tree-optimization/89595
> > * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> > stmt iterator as reference, take boolean output parameter to
> > indicate whether the stmt was removed and thus the iterator
> > already advanced.
> > (dom_opt_dom_walker::before_dom_children): Re-iterate over
> > stmts created by folding.
> >
> > * gcc.dg/torture/pr89595.c: New testcase.
> >
>
> Well, all the real logic changs are in the before_dom_children method.
> The bits in optimize_stmt are trivial enough to effectively ignore.
>
> I don't see a better way to discover and process statements that are
> created in the bowels of fold_stmt.

I'm not entirely happy so I created the following alternative which
is a bit larger and slower due to the pre-pass clearing the visited flag
but is IMHO easier to follow.  I guess there's plenty of TLC opportunity
here but then I also hope to retire the VN parts of DOM in favor
of the non-iterating RPO-VN code...

So - I'd lean to this variant even though it has the extra loop over stmts,
would you agree?

Bootstrap / regtest running on x86_64-unknown-linux-gnu.

Richard.

2019-03-06  Richard Biener  

PR tree-optimization/89595
* tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
stmt iterator as reference, take boolean output parameter to
indicate whether the stmt was removed and thus the iterator
already advanced.
(dom_opt_dom_walker::before_dom_children): Re-iterate over
stmts created by folding.

* gcc.dg/torture/pr89595.c: New testcase.


fix-pr89595-2
Description: Binary data

Re: A bug in vrp_meet?

[Jeff Law]

On 3/5/19 7:44 AM, Richard Biener wrote:

> So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> the MAX_EXPR introduced by folding makes it somewhat ugly.
> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> 
> Any ideas how to make it less so?  I can split out making optimize_stmt
> take a gsi * btw, in case that's a more obvious change and it makes the
> patch a little smaller.
> 
> Richard.
> 
> 2019-03-05  Richard Biener  
> 
> PR tree-optimization/89595
> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> stmt iterator as reference, take boolean output parameter to
> indicate whether the stmt was removed and thus the iterator
> already advanced.
> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> stmts created by folding.
> 
> * gcc.dg/torture/pr89595.c: New testcase.
> 

Well, all the real logic changs are in the before_dom_children method.
The bits in optimize_stmt are trivial enough to effectively ignore.

I don't see a better way to discover and process statements that are
created in the bowels of fold_stmt.

Jeff

Re: A bug in vrp_meet?

[Jeff Law]

On 3/5/19 7:44 AM, Richard Biener wrote:

> So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> the MAX_EXPR introduced by folding makes it somewhat ugly.
> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> 
> Any ideas how to make it less so?  I can split out making optimize_stmt
> take a gsi * btw, in case that's a more obvious change and it makes the
> patch a little smaller.
> 
> Richard.
> 
> 2019-03-05  Richard Biener  
> 
> PR tree-optimization/89595
> * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
> stmt iterator as reference, take boolean output parameter to
> indicate whether the stmt was removed and thus the iterator
> already advanced.
> (dom_opt_dom_walker::before_dom_children): Re-iterate over
> stmts created by folding.
> 
> * gcc.dg/torture/pr89595.c: New testcase.
> 

Well, all the real logic changs are in the before_dom_children method.
The bits in optimize_stmt are trivial enough to effectively ignore.

I don't see a better way to discover and process statements that are
created in the bowels of fold_stmt.

Jeff

Re: A bug in vrp_meet?

[Jeff Law]

On 3/4/19 4:45 AM, Richard Biener wrote:
> On Fri, Mar 1, 2019 at 10:02 PM Qing Zhao  wrote:
>>
>>
>> On Mar 1, 2019, at 1:25 PM, Richard Biener  
>> wrote:
>>
>> On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  
>> wrote:
>>
>> Jeff,
>>
>> thanks a lot for the reply.
>>
>> this is really helpful.
>>
>> I double checked the dumped intermediate file for pass “dom3", and
>> located the following for _152:
>>
>> BEFORE the pass “dom3”, there is no _152, the corresponding Block
>> looks like:
>>
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = (sword) ufcMSR_52(D);
>> i_49 = _98 > 0 ? k_105 : 0;
>>
>> ***During the pass “doms”,  _152 is generated as following:
>>
>> Optimizing block #4
>> ….
>> 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
>>
>> then bb 4 becomes:
>>
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = _98;
>> _152 = MAX_EXPR <_98, 0>;
>> i_49 = _152;
>>
>> and all the i_49 are replaced with _152.
>>
>> However, the value range info for _152 doesnot reflect the one for
>> i_49, it keeps as UNDEFINED.
>>
>> is this the root problem?
>>
>>
>> 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 ())
> -{
> -  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, );
> +   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).
Grr. This all sounds very familiar.  I know I've tracked down a bug of
this  nature before and would like to review how it was fixed in light
of your comment about tree-ssa-forwprop's handling of the same
situation.  I just can't seem to find it..  Ugh.

I assume you're referring to the PLF stuff?


Jeff

Re: A bug in vrp_meet?

[Jeff Law]

On 3/4/19 4:45 AM, Richard Biener wrote:
> On Fri, Mar 1, 2019 at 10:02 PM Qing Zhao  wrote:
>>
>>
>> On Mar 1, 2019, at 1:25 PM, Richard Biener  
>> wrote:
>>
>> On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  
>> wrote:
>>
>> Jeff,
>>
>> thanks a lot for the reply.
>>
>> this is really helpful.
>>
>> I double checked the dumped intermediate file for pass “dom3", and
>> located the following for _152:
>>
>> BEFORE the pass “dom3”, there is no _152, the corresponding Block
>> looks like:
>>
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = (sword) ufcMSR_52(D);
>> i_49 = _98 > 0 ? k_105 : 0;
>>
>> ***During the pass “doms”,  _152 is generated as following:
>>
>> Optimizing block #4
>> ….
>> 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
>>
>> then bb 4 becomes:
>>
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = _98;
>> _152 = MAX_EXPR <_98, 0>;
>> i_49 = _152;
>>
>> and all the i_49 are replaced with _152.
>>
>> However, the value range info for _152 doesnot reflect the one for
>> i_49, it keeps as UNDEFINED.
>>
>> is this the root problem?
>>
>>
>> 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 ())
> -{
> -  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, );
> +   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).
Grr. This all sounds very familiar.  I know I've tracked down a bug of
this  nature before and would like to review how it was fixed in light
of your comment about tree-ssa-forwprop's handling of the same
situation.  I just can't seem to find it..  Ugh.

I assume you're referring to the PLF stuff?


Jeff

Re: A bug in vrp_meet?

[Jeff Law]

On 3/1/19 10:49 AM, Qing Zhao wrote:
> Jeff,
> 
> thanks a lot for the reply.
> 
> this is really helpful.
> 
> I double checked the dumped intermediate file for pass “dom3", and
> located the following for _152:
> 
> BEFORE the pass “dom3”, there is no _152, the corresponding Block
> looks like:
> 
>    [local count: 12992277]:
>   _98 = (int) ufcMSR_52(D);
>   k_105 = (sword) ufcMSR_52(D);
>   i_49 = _98 > 0 ? k_105 : 0;
> 
> ***During the pass “doms”,  _152 is generated as following:
> 
> Optimizing block #4
> ….
> 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
> 
> then bb 4 becomes:
> 
>    [local count: 12992277]:
>   _98 = (int) ufcMSR_52(D);
>   k_105 = _98;
>   _152 = MAX_EXPR <_98, 0>;
>   i_49 = _152;
> 
> and all the i_49 are replaced with _152. 
> 
> However, the value range info for _152 doesnot reflect the one for i_49,
> it keeps as UNDEFINED. 
> 
> is this the root problem?  
Yes, I think so.  It's like we didn't record anything for _152 when we
simplified the RHS to a MAX_EXPR.

Jeff

Re: A bug in vrp_meet?

[Jeff Law]

On 3/1/19 10:49 AM, Qing Zhao wrote:
> Jeff,
> 
> thanks a lot for the reply.
> 
> this is really helpful.
> 
> I double checked the dumped intermediate file for pass “dom3", and
> located the following for _152:
> 
> BEFORE the pass “dom3”, there is no _152, the corresponding Block
> looks like:
> 
>    [local count: 12992277]:
>   _98 = (int) ufcMSR_52(D);
>   k_105 = (sword) ufcMSR_52(D);
>   i_49 = _98 > 0 ? k_105 : 0;
> 
> ***During the pass “doms”,  _152 is generated as following:
> 
> Optimizing block #4
> ….
> 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
> 
> then bb 4 becomes:
> 
>    [local count: 12992277]:
>   _98 = (int) ufcMSR_52(D);
>   k_105 = _98;
>   _152 = MAX_EXPR <_98, 0>;
>   i_49 = _152;
> 
> and all the i_49 are replaced with _152. 
> 
> However, the value range info for _152 doesnot reflect the one for i_49,
> it keeps as UNDEFINED. 
> 
> is this the root problem?  
Yes, I think so.  It's like we didn't record anything for _152 when we
simplified the RHS to a MAX_EXPR.

Jeff

Re: A bug in vrp_meet?

[Qing Zhao]

> On Mar 5, 2019, at 8:44 AM, Richard Biener  wrote:
> 
 
 
 will you commit this fix to gcc9 and gcc8  (we need it in gcc8)?
>>> 
>>> I'll see to carve out some cycles trying to find a testcase and amend
>>> the fix a bit
>>> and will take care of testing/submitting the fix.  Thanks for testing
>>> that it works
>>> for your case.
>> 
>> I filed PR89595 with a testcase.
> 
> So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
> the MAX_EXPR introduced by folding makes it somewhat ugly.

I tried the new patch on my side,Now, in the dump file of 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>;
Optimizing statement _152 = MAX_EXPR <_98, 0>;
LKUP STMT _152 = _98 max_expr 0
2>>> STMT _152 = _98 max_expr 0


thanks.

Qing

> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> 
> Any ideas how to make it less so?  I can split out making optimize_stmt
> take a gsi * btw, in case that's a more obvious change and it makes the
> patch a little smaller.
> 
> Richard.
> 
> 2019-03-05  Richard Biener  
> 
>PR tree-optimization/89595
>* tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Take
>stmt iterator as reference, take boolean output parameter to
>indicate whether the stmt was removed and thus the iterator
>already advanced.
>(dom_opt_dom_walker::before_dom_children): Re-iterate over
>stmts created by folding.
> 
>* gcc.dg/torture/pr89595.c: New testcase.
> 

Re: A bug in vrp_meet?

[Qing Zhao]

> On Mar 5, 2019, at 4:44 AM, Richard Biener  wrote:
> 
>>> 
>>> will you commit this fix to gcc9 and gcc8  (we need it in gcc8)?
>> 
>> I'll see to carve out some cycles trying to find a testcase and amend
>> the fix a bit
>> and will take care of testing/submitting the fix.  Thanks for testing
>> that it works
>> for your case.
> 
> I filed PR89595 with a testcase.

Thanks a lot for the bug and the testing case. 

it’s good to know that it’s possible to write a test case with GIMPLE in gcc.

Qing
> 
> Richard.

Re: A bug in vrp_meet?

[Richard Biener]

On Tue, Mar 5, 2019 at 11:44 AM Richard Biener
 wrote:
>
> On Tue, Mar 5, 2019 at 10:48 AM Richard Biener
>  wrote:
> >
> > On Mon, Mar 4, 2019 at 11:01 PM Qing Zhao  wrote:
> > >
> > > Hi, Richard,
> > >
> > > > On Mar 4, 2019, at 5:45 AM, Richard Biener  
> > > > 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 ())
> > > > -{
> > > > -  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, );
> > > > +   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;
> >
> > Ah, that looks interesting.  From this detail we might be
> > able to derive a testcase as well - a GIMPLE one
> > eventually because DOM runs quite late.  It's also interesting
> > to see the inefficient code here (the extra copy), probably
> > some known issue with match-and-simplify, I'd have to check.
> >
> > >   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.
> >
> > We can also see that DOMs optimize_stmt code is not executed on the first 
> > stmt
> > of the folding result (the MAX_EXPR), so the fix can be probably
> > amended/simplified
> > with that in mind.
> >
> > > 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)?
> >
> > I'll see to carve out some cycles trying to find a testcase and amend
> > the fix a bit
> > and will take care of testing/submitting the fix.  Thanks for testing
> > that it works
> > for your case.
>
> I filed PR89595 with a testcase.

So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
the MAX_EXPR introduced by folding makes it somewhat ugly.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Any ideas how to make it less so?  I can split out making optimize_stmt
take a gsi * btw, in case that's a more obvious change and it makes the
patch a little smaller.

Richard.

2019-03-05  Richard Biener  

   

Re: A bug in vrp_meet?

[Richard Biener]

On Tue, Mar 5, 2019 at 11:44 AM Richard Biener
 wrote:
>
> On Tue, Mar 5, 2019 at 10:48 AM Richard Biener
>  wrote:
> >
> > On Mon, Mar 4, 2019 at 11:01 PM Qing Zhao  wrote:
> > >
> > > Hi, Richard,
> > >
> > > > On Mar 4, 2019, at 5:45 AM, Richard Biener  
> > > > 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 ())
> > > > -{
> > > > -  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, );
> > > > +   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;
> >
> > Ah, that looks interesting.  From this detail we might be
> > able to derive a testcase as well - a GIMPLE one
> > eventually because DOM runs quite late.  It's also interesting
> > to see the inefficient code here (the extra copy), probably
> > some known issue with match-and-simplify, I'd have to check.
> >
> > >   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.
> >
> > We can also see that DOMs optimize_stmt code is not executed on the first 
> > stmt
> > of the folding result (the MAX_EXPR), so the fix can be probably
> > amended/simplified
> > with that in mind.
> >
> > > 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)?
> >
> > I'll see to carve out some cycles trying to find a testcase and amend
> > the fix a bit
> > and will take care of testing/submitting the fix.  Thanks for testing
> > that it works
> > for your case.
>
> I filed PR89595 with a testcase.

So fixing it properly with also re-optimize_stmt those stmts so we'd CSE
the MAX_EXPR introduced by folding makes it somewhat ugly.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Any ideas how to make it less so?  I can split out making optimize_stmt
take a gsi * btw, in case that's a more obvious change and it makes the
patch a little smaller.

Richard.

2019-03-05  Richard Biener  

   

Re: A bug in vrp_meet?

[Richard Biener]

On Tue, Mar 5, 2019 at 10:48 AM Richard Biener
 wrote:
>
> On Mon, Mar 4, 2019 at 11:01 PM Qing Zhao  wrote:
> >
> > Hi, Richard,
> >
> > > On Mar 4, 2019, at 5:45 AM, Richard Biener  
> > > 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 ())
> > > -{
> > > -  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, );
> > > +   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;
>
> Ah, that looks interesting.  From this detail we might be
> able to derive a testcase as well - a GIMPLE one
> eventually because DOM runs quite late.  It's also interesting
> to see the inefficient code here (the extra copy), probably
> some known issue with match-and-simplify, I'd have to check.
>
> >   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.
>
> We can also see that DOMs optimize_stmt code is not executed on the first stmt
> of the folding result (the MAX_EXPR), so the fix can be probably
> amended/simplified
> with that in mind.
>
> > 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)?
>
> I'll see to carve out some cycles trying to find a testcase and amend
> the fix a bit
> and will take care of testing/submitting the fix.  Thanks for testing
> that it works
> for your case.

I filed PR89595 with a testcase.

Richard.

> Richard.
>
> > 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 ())
> > > {
> > > +  gimple_stmt_iterator pgsi = gsi;
> > > +  gsi_prev ();
> > >   evrp_range_analyzer.record_ranges_from_stmt (gsi_stmt (gsi), false);
> > >   taken_edge = this->optimize_stmt (bb, gsi);
> > > +  gimple_stmt_iterator 

Re: A bug in vrp_meet?

[Richard Biener]

On Tue, Mar 5, 2019 at 10:48 AM Richard Biener
 wrote:
>
> On Mon, Mar 4, 2019 at 11:01 PM Qing Zhao  wrote:
> >
> > Hi, Richard,
> >
> > > On Mar 4, 2019, at 5:45 AM, Richard Biener  
> > > 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 ())
> > > -{
> > > -  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, );
> > > +   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;
>
> Ah, that looks interesting.  From this detail we might be
> able to derive a testcase as well - a GIMPLE one
> eventually because DOM runs quite late.  It's also interesting
> to see the inefficient code here (the extra copy), probably
> some known issue with match-and-simplify, I'd have to check.
>
> >   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.
>
> We can also see that DOMs optimize_stmt code is not executed on the first stmt
> of the folding result (the MAX_EXPR), so the fix can be probably
> amended/simplified
> with that in mind.
>
> > 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)?
>
> I'll see to carve out some cycles trying to find a testcase and amend
> the fix a bit
> and will take care of testing/submitting the fix.  Thanks for testing
> that it works
> for your case.

I filed PR89595 with a testcase.

Richard.

> Richard.
>
> > 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 ())
> > > {
> > > +  gimple_stmt_iterator pgsi = gsi;
> > > +  gsi_prev ();
> > >   evrp_range_analyzer.record_ranges_from_stmt (gsi_stmt (gsi), false);
> > >   taken_edge = this->optimize_stmt (bb, gsi);
> > > +  gimple_stmt_iterator 

Re: A bug in vrp_meet?

[Richard Biener]

On Mon, Mar 4, 2019 at 11:01 PM Qing Zhao  wrote:
>
> Hi, Richard,
>
> > On Mar 4, 2019, at 5:45 AM, Richard Biener  
> > 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 ())
> > -{
> > -  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, );
> > +   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;

Ah, that looks interesting.  From this detail we might be
able to derive a testcase as well - a GIMPLE one
eventually because DOM runs quite late.  It's also interesting
to see the inefficient code here (the extra copy), probably
some known issue with match-and-simplify, I'd have to check.

>   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.

We can also see that DOMs optimize_stmt code is not executed on the first stmt
of the folding result (the MAX_EXPR), so the fix can be probably
amended/simplified
with that in mind.

> 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)?

I'll see to carve out some cycles trying to find a testcase and amend
the fix a bit
and will take care of testing/submitting the fix.  Thanks for testing
that it works
for your case.

Richard.

> 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 ())
> > {
> > +  gimple_stmt_iterator pgsi = gsi;
> > +  gsi_prev ();
> >   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 ();
> > +  /* 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
> > + 

Re: A bug in vrp_meet?

[Richard Biener]

On Mon, Mar 4, 2019 at 11:01 PM Qing Zhao  wrote:
>
> Hi, Richard,
>
> > On Mar 4, 2019, at 5:45 AM, Richard Biener  
> > 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 ())
> > -{
> > -  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, );
> > +   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;

Ah, that looks interesting.  From this detail we might be
able to derive a testcase as well - a GIMPLE one
eventually because DOM runs quite late.  It's also interesting
to see the inefficient code here (the extra copy), probably
some known issue with match-and-simplify, I'd have to check.

>   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.

We can also see that DOMs optimize_stmt code is not executed on the first stmt
of the folding result (the MAX_EXPR), so the fix can be probably
amended/simplified
with that in mind.

> 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)?

I'll see to carve out some cycles trying to find a testcase and amend
the fix a bit
and will take care of testing/submitting the fix.  Thanks for testing
that it works
for your case.

Richard.

> 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 ())
> > {
> > +  gimple_stmt_iterator pgsi = gsi;
> > +  gsi_prev ();
> >   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 ();
> > +  /* 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
> > + 

Re: A bug in vrp_meet?

[Qing Zhao]

Hi, Richard,

> On Mar 4, 2019, at 5:45 AM, Richard Biener  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 ())
> -{
> -  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, );
> +   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 ())
> {
> +  gimple_stmt_iterator pgsi = gsi;
> +  gsi_prev ();
>   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 ();
> +  /* 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 ();
> +   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.
>> 
>> 

Re: A bug in vrp_meet?

[Qing Zhao]

Hi, Richard,

> On Mar 4, 2019, at 5:45 AM, Richard Biener  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 ())
> -{
> -  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, );
> +   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 ())
> {
> +  gimple_stmt_iterator pgsi = gsi;
> +  gsi_prev ();
>   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 ();
> +  /* 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 ();
> +   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.
>> 
>> 

Re: A bug in vrp_meet?

[Qing Zhao]

Richard,

thanks a lot for your suggested fix. 

I will try it.

Qing
> On Mar 4, 2019, at 5:45 AM, Richard Biener  wrote:
> 
> On Fri, Mar 1, 2019 at 10:02 PM Qing Zhao  wrote:
>> 
>> 
>> On Mar 1, 2019, at 1:25 PM, Richard Biener  
>> wrote:
>> 
>> On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  
>> wrote:
>> 
>> Jeff,
>> 
>> thanks a lot for the reply.
>> 
>> this is really helpful.
>> 
>> I double checked the dumped intermediate file for pass “dom3", and
>> located the following for _152:
>> 
>> BEFORE the pass “dom3”, there is no _152, the corresponding Block
>> looks like:
>> 
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = (sword) ufcMSR_52(D);
>> i_49 = _98 > 0 ? k_105 : 0;
>> 
>> ***During the pass “doms”,  _152 is generated as following:
>> 
>> Optimizing block #4
>> ….
>> 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
>> 
>> then bb 4 becomes:
>> 
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = _98;
>> _152 = MAX_EXPR <_98, 0>;
>> i_49 = _152;
>> 
>> and all the i_49 are replaced with _152.
>> 
>> However, the value range info for _152 doesnot reflect the one for
>> i_49, it keeps as UNDEFINED.
>> 
>> is this the root problem?
>> 
>> 
>> 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 ())
> -{
> -  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, );
> +   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?
> 
> 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 ())
> {
> +  gimple_stmt_iterator pgsi = gsi;
> +  gsi_prev ();
>   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 ();
> +  /* 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 ();
> +   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.
>> 
>> 

Re: A bug in vrp_meet?

[Qing Zhao]

Richard,

thanks a lot for your suggested fix. 

I will try it.

Qing
> On Mar 4, 2019, at 5:45 AM, Richard Biener  wrote:
> 
> On Fri, Mar 1, 2019 at 10:02 PM Qing Zhao  wrote:
>> 
>> 
>> On Mar 1, 2019, at 1:25 PM, Richard Biener  
>> wrote:
>> 
>> On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  
>> wrote:
>> 
>> Jeff,
>> 
>> thanks a lot for the reply.
>> 
>> this is really helpful.
>> 
>> I double checked the dumped intermediate file for pass “dom3", and
>> located the following for _152:
>> 
>> BEFORE the pass “dom3”, there is no _152, the corresponding Block
>> looks like:
>> 
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = (sword) ufcMSR_52(D);
>> i_49 = _98 > 0 ? k_105 : 0;
>> 
>> ***During the pass “doms”,  _152 is generated as following:
>> 
>> Optimizing block #4
>> ….
>> 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
>> 
>> then bb 4 becomes:
>> 
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = _98;
>> _152 = MAX_EXPR <_98, 0>;
>> i_49 = _152;
>> 
>> and all the i_49 are replaced with _152.
>> 
>> However, the value range info for _152 doesnot reflect the one for
>> i_49, it keeps as UNDEFINED.
>> 
>> is this the root problem?
>> 
>> 
>> 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 ())
> -{
> -  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, );
> +   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?
> 
> 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 ())
> {
> +  gimple_stmt_iterator pgsi = gsi;
> +  gsi_prev ();
>   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 ();
> +  /* 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 ();
> +   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.
>> 
>> 

Re: A bug in vrp_meet?

[Richard Biener]

On Fri, Mar 1, 2019 at 10:02 PM Qing Zhao  wrote:
>
>
> On Mar 1, 2019, at 1:25 PM, Richard Biener  wrote:
>
> On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  wrote:
>
> Jeff,
>
> thanks a lot for the reply.
>
> this is really helpful.
>
> I double checked the dumped intermediate file for pass “dom3", and
> located the following for _152:
>
> BEFORE the pass “dom3”, there is no _152, the corresponding Block
> looks like:
>
>  [local count: 12992277]:
> _98 = (int) ufcMSR_52(D);
> k_105 = (sword) ufcMSR_52(D);
> i_49 = _98 > 0 ? k_105 : 0;
>
> ***During the pass “doms”,  _152 is generated as following:
>
> Optimizing block #4
> ….
> 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
>
> then bb 4 becomes:
>
>  [local count: 12992277]:
> _98 = (int) ufcMSR_52(D);
> k_105 = _98;
> _152 = MAX_EXPR <_98, 0>;
> i_49 = _152;
>
> and all the i_49 are replaced with _152.
>
> However, the value range info for _152 doesnot reflect the one for
> i_49, it keeps as UNDEFINED.
>
> is this the root problem?
>
>
> 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 ())
-{
-  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, );
+   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?

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 ())
 {
+  gimple_stmt_iterator pgsi = gsi;
+  gsi_prev ();
   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 ();
+  /* 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 ();
+   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.
>
>

Re: A bug in vrp_meet?

[Richard Biener]

On Fri, Mar 1, 2019 at 10:02 PM Qing Zhao  wrote:
>
>
> On Mar 1, 2019, at 1:25 PM, Richard Biener  wrote:
>
> On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  wrote:
>
> Jeff,
>
> thanks a lot for the reply.
>
> this is really helpful.
>
> I double checked the dumped intermediate file for pass “dom3", and
> located the following for _152:
>
> BEFORE the pass “dom3”, there is no _152, the corresponding Block
> looks like:
>
>  [local count: 12992277]:
> _98 = (int) ufcMSR_52(D);
> k_105 = (sword) ufcMSR_52(D);
> i_49 = _98 > 0 ? k_105 : 0;
>
> ***During the pass “doms”,  _152 is generated as following:
>
> Optimizing block #4
> ….
> 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
>
> then bb 4 becomes:
>
>  [local count: 12992277]:
> _98 = (int) ufcMSR_52(D);
> k_105 = _98;
> _152 = MAX_EXPR <_98, 0>;
> i_49 = _152;
>
> and all the i_49 are replaced with _152.
>
> However, the value range info for _152 doesnot reflect the one for
> i_49, it keeps as UNDEFINED.
>
> is this the root problem?
>
>
> 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 ())
-{
-  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, );
+   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?

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 ())
 {
+  gimple_stmt_iterator pgsi = gsi;
+  gsi_prev ();
   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 ();
+  /* 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 ();
+   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.
>
>

Re: A bug in vrp_meet?

[Qing Zhao]

> On Mar 1, 2019, at 1:25 PM, Richard Biener  wrote:
> 
> On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  > wrote:
>> Jeff,
>> 
>> thanks a lot for the reply.
>> 
>> this is really helpful.
>> 
>> I double checked the dumped intermediate file for pass “dom3", and
>> located the following for _152:
>> 
>> BEFORE the pass “dom3”, there is no _152, the corresponding Block
>> looks like:
>> 
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = (sword) ufcMSR_52(D);
>> i_49 = _98 > 0 ? k_105 : 0;
>> 
>> ***During the pass “doms”,  _152 is generated as following:
>> 
>> Optimizing block #4
>> ….
>> 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
>> 
>> then bb 4 becomes:
>> 
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = _98;
>> _152 = MAX_EXPR <_98, 0>;
>> i_49 = _152;
>> 
>> and all the i_49 are replaced with _152. 
>> 
>> However, the value range info for _152 doesnot reflect the one for
>> i_49, it keeps as UNDEFINED. 
>> 
>> is this the root problem?  
> 
> 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?

Thanks a lot.

Qing


> 
> Richard. 
> 

Re: A bug in vrp_meet?

[Qing Zhao]

> On Mar 1, 2019, at 1:25 PM, Richard Biener  wrote:
> 
> On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  > wrote:
>> Jeff,
>> 
>> thanks a lot for the reply.
>> 
>> this is really helpful.
>> 
>> I double checked the dumped intermediate file for pass “dom3", and
>> located the following for _152:
>> 
>> BEFORE the pass “dom3”, there is no _152, the corresponding Block
>> looks like:
>> 
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = (sword) ufcMSR_52(D);
>> i_49 = _98 > 0 ? k_105 : 0;
>> 
>> ***During the pass “doms”,  _152 is generated as following:
>> 
>> Optimizing block #4
>> ….
>> 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
>> 
>> then bb 4 becomes:
>> 
>>  [local count: 12992277]:
>> _98 = (int) ufcMSR_52(D);
>> k_105 = _98;
>> _152 = MAX_EXPR <_98, 0>;
>> i_49 = _152;
>> 
>> and all the i_49 are replaced with _152. 
>> 
>> However, the value range info for _152 doesnot reflect the one for
>> i_49, it keeps as UNDEFINED. 
>> 
>> is this the root problem?  
> 
> 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?

Thanks a lot.

Qing


> 
> Richard. 
> 

Re: A bug in vrp_meet?

[Richard Biener]

On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  wrote:
>Jeff,
>
>thanks a lot for the reply.
>
>this is really helpful.
>
>I double checked the dumped intermediate file for pass “dom3", and
>located the following for _152:
>
>BEFORE the pass “dom3”, there is no _152, the corresponding Block
>looks like:
>
>   [local count: 12992277]:
>  _98 = (int) ufcMSR_52(D);
>  k_105 = (sword) ufcMSR_52(D);
>  i_49 = _98 > 0 ? k_105 : 0;
>
>***During the pass “doms”,  _152 is generated as following:
>
>Optimizing block #4
>….
>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
>
>then bb 4 becomes:
>
>   [local count: 12992277]:
>  _98 = (int) ufcMSR_52(D);
>  k_105 = _98;
>  _152 = MAX_EXPR <_98, 0>;
>  i_49 = _152;
>
>and all the i_49 are replaced with _152. 
>
>However, the value range info for _152 doesnot reflect the one for
>i_49, it keeps as UNDEFINED. 
>
>is this the root problem?  

It looks like DOM fails to visit stmts generated by simplification. Can you 
open a bug report with a testcase?

Richard. 

>thanks a lot.
>
>Qing
>
>> On Feb 28, 2019, at 1:54 PM, Jeff Law  wrote:
>> 
>> On 2/28/19 10:05 AM, Qing Zhao wrote:
>>> Hi,
>>> 
>>> I have been debugging a runtime error caused by value range
>propagation. and finally located to the following gcc routine:
>>> 
>>> vrp_meet_1 in gcc/tree-vrp.c
>>> 
>>> 
>>> /* Meet operation for value ranges.  Given two value ranges VR0 and
>>>   VR1, store in VR0 a range that contains both VR0 and VR1.  This
>>>   may not be the smallest possible such range.  */
>>> 
>>> static void 
>>> vrp_meet_1 (value_range *vr0, const value_range *vr1)
>>> {
>>>  value_range saved;
>>> 
>>>  if (vr0->type == VR_UNDEFINED)
>>>{
>>>  set_value_range (vr0, vr1->type, vr1->min, vr1->max,
>vr1->equiv);
>>>  return;
>>>}
>>> 
>>>  if (vr1->type == VR_UNDEFINED)
>>>{
>>>  /* VR0 already has the resulting range.  */
>>>  return;
>>>}
>>> 
>>> 
>>> In the above, when one of vr0 or vr1 is VR_UNDEFINED,  the meet
>result of these two will be  the other VALUE. 
>>> 
>>> This seems not correct to me. 
>> That's the optimistic nature of VRP.  It's generally desired
>behavior.
>> 
>>> 
>>> For example, the following is the located incorrect value range
>propagation:  (portion from the dump file *.181t.dom3)
>>> 
>>> 
>>> Visiting PHI node: i_83 = PHI <_152(20), 0(22)>
>>>Argument #0 (20 -> 10 executable)
>>>_152: UNDEFINED
>>>Argument #1 (22 -> 10 executable)
>>>0: [0, 0]
>>> Meeting
>>>  UNDEFINED
>>> and
>>>  [0, 0]
>>> to
>>>  [0, 0]
>>> Intersecting
>>>  [0, 0]
>>> and
>>>  [0, 65535]
>>> to
>>>  [0, 0]
>>> 
>>> 
>>> 
>>> In the above, “i_83” is defined as PHI <_152(20), 0(22)>,   the 1st
>argument is UNDEFINED at this time(but its value range definitely is
>NOT [0,0]),
>>> and the 2nd argument is 0.
>> If it's value is undefined then it can be any value we want.  We
>choose
>> to make it equal to the other argument.
>> 
>> If VRP later finds that _152 changes, then it will go back and
>> reevaluate the PHI.  That's one of the basic design principles of the
>> optimistic propagators.
>> 
>>> 
>>> “vrp_meet” generate a VR_RANGE with [0,0] for “i_83” based on the
>current algorithm.  Obviously, this result VR_RANGE with [0,0] does NOT
>
>>> contain the value ranges for _152. 
>>> 
>>> the result of “vrp_meet” is Not correct.  and this incorrect value
>range result finally caused the runtime error. 
>>> 
>>> I ‘d like to modify the vrp_meet_1 as following:
>>> 
>>> 
>>> static void 
>>> vrp_meet_1 (value_range *vr0, const value_range *vr1)
>>> {
>>>  value_range saved;
>>> 
>>>  if (vr0->type == VR_UNDEFINED)
>>>{
>>>  /* VR0 already has the resulting range. */
>>>  return;
>>>}
>>> 
>>>  if (vr1->type == VR_UNDEFINED)
>>>{
>>>  set_value_range_to_undefined (vr0)
>>> return;
>>>}
>>> 
>>> 
>>> let me know your opinion.
>>> 
>>> thanks a lot for the help.
>> I think we (Richi and I) went through this about a year ago and the
>> conclusion was we should be looking at how you're getting into the
>> vrp_meet with the VR_UNDEFINED.
>> 
>> If it happens because the user's code has an undefined use, then, the
>> consensus has been to go ahead and optimize it.
>> 
>> If it happens for any other reason, then it's likely a bug in GCC. 
>We
>> had a couple of these when we made EVRP a re-usable module and
>started
>> exploiting its data in the jump threader.
>> 
>> So you need to work backwards from this point to figure out how you
>got
>> here.
>> 
>> jeff

Re: A bug in vrp_meet?

[Richard Biener]

On March 1, 2019 6:49:20 PM GMT+01:00, Qing Zhao  wrote:
>Jeff,
>
>thanks a lot for the reply.
>
>this is really helpful.
>
>I double checked the dumped intermediate file for pass “dom3", and
>located the following for _152:
>
>BEFORE the pass “dom3”, there is no _152, the corresponding Block
>looks like:
>
>   [local count: 12992277]:
>  _98 = (int) ufcMSR_52(D);
>  k_105 = (sword) ufcMSR_52(D);
>  i_49 = _98 > 0 ? k_105 : 0;
>
>***During the pass “doms”,  _152 is generated as following:
>
>Optimizing block #4
>….
>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
>
>then bb 4 becomes:
>
>   [local count: 12992277]:
>  _98 = (int) ufcMSR_52(D);
>  k_105 = _98;
>  _152 = MAX_EXPR <_98, 0>;
>  i_49 = _152;
>
>and all the i_49 are replaced with _152. 
>
>However, the value range info for _152 doesnot reflect the one for
>i_49, it keeps as UNDEFINED. 
>
>is this the root problem?  

It looks like DOM fails to visit stmts generated by simplification. Can you 
open a bug report with a testcase?

Richard. 

>thanks a lot.
>
>Qing
>
>> On Feb 28, 2019, at 1:54 PM, Jeff Law  wrote:
>> 
>> On 2/28/19 10:05 AM, Qing Zhao wrote:
>>> Hi,
>>> 
>>> I have been debugging a runtime error caused by value range
>propagation. and finally located to the following gcc routine:
>>> 
>>> vrp_meet_1 in gcc/tree-vrp.c
>>> 
>>> 
>>> /* Meet operation for value ranges.  Given two value ranges VR0 and
>>>   VR1, store in VR0 a range that contains both VR0 and VR1.  This
>>>   may not be the smallest possible such range.  */
>>> 
>>> static void 
>>> vrp_meet_1 (value_range *vr0, const value_range *vr1)
>>> {
>>>  value_range saved;
>>> 
>>>  if (vr0->type == VR_UNDEFINED)
>>>{
>>>  set_value_range (vr0, vr1->type, vr1->min, vr1->max,
>vr1->equiv);
>>>  return;
>>>}
>>> 
>>>  if (vr1->type == VR_UNDEFINED)
>>>{
>>>  /* VR0 already has the resulting range.  */
>>>  return;
>>>}
>>> 
>>> 
>>> In the above, when one of vr0 or vr1 is VR_UNDEFINED,  the meet
>result of these two will be  the other VALUE. 
>>> 
>>> This seems not correct to me. 
>> That's the optimistic nature of VRP.  It's generally desired
>behavior.
>> 
>>> 
>>> For example, the following is the located incorrect value range
>propagation:  (portion from the dump file *.181t.dom3)
>>> 
>>> 
>>> Visiting PHI node: i_83 = PHI <_152(20), 0(22)>
>>>Argument #0 (20 -> 10 executable)
>>>_152: UNDEFINED
>>>Argument #1 (22 -> 10 executable)
>>>0: [0, 0]
>>> Meeting
>>>  UNDEFINED
>>> and
>>>  [0, 0]
>>> to
>>>  [0, 0]
>>> Intersecting
>>>  [0, 0]
>>> and
>>>  [0, 65535]
>>> to
>>>  [0, 0]
>>> 
>>> 
>>> 
>>> In the above, “i_83” is defined as PHI <_152(20), 0(22)>,   the 1st
>argument is UNDEFINED at this time(but its value range definitely is
>NOT [0,0]),
>>> and the 2nd argument is 0.
>> If it's value is undefined then it can be any value we want.  We
>choose
>> to make it equal to the other argument.
>> 
>> If VRP later finds that _152 changes, then it will go back and
>> reevaluate the PHI.  That's one of the basic design principles of the
>> optimistic propagators.
>> 
>>> 
>>> “vrp_meet” generate a VR_RANGE with [0,0] for “i_83” based on the
>current algorithm.  Obviously, this result VR_RANGE with [0,0] does NOT
>
>>> contain the value ranges for _152. 
>>> 
>>> the result of “vrp_meet” is Not correct.  and this incorrect value
>range result finally caused the runtime error. 
>>> 
>>> I ‘d like to modify the vrp_meet_1 as following:
>>> 
>>> 
>>> static void 
>>> vrp_meet_1 (value_range *vr0, const value_range *vr1)
>>> {
>>>  value_range saved;
>>> 
>>>  if (vr0->type == VR_UNDEFINED)
>>>{
>>>  /* VR0 already has the resulting range. */
>>>  return;
>>>}
>>> 
>>>  if (vr1->type == VR_UNDEFINED)
>>>{
>>>  set_value_range_to_undefined (vr0)
>>> return;
>>>}
>>> 
>>> 
>>> let me know your opinion.
>>> 
>>> thanks a lot for the help.
>> I think we (Richi and I) went through this about a year ago and the
>> conclusion was we should be looking at how you're getting into the
>> vrp_meet with the VR_UNDEFINED.
>> 
>> If it happens because the user's code has an undefined use, then, the
>> consensus has been to go ahead and optimize it.
>> 
>> If it happens for any other reason, then it's likely a bug in GCC. 
>We
>> had a couple of these when we made EVRP a re-usable module and
>started
>> exploiting its data in the jump threader.
>> 
>> So you need to work backwards from this point to figure out how you
>got
>> here.
>> 
>> jeff

Re: A bug in vrp_meet?

[Qing Zhao]

Jeff,

thanks a lot for the reply.

this is really helpful.

I double checked the dumped intermediate file for pass “dom3", and located the 
following for _152:

BEFORE the pass “dom3”, there is no _152, the corresponding Block looks 
like:

   [local count: 12992277]:
  _98 = (int) ufcMSR_52(D);
  k_105 = (sword) ufcMSR_52(D);
  i_49 = _98 > 0 ? k_105 : 0;

***During the pass “doms”,  _152 is generated as following:

Optimizing block #4
….
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

then bb 4 becomes:

   [local count: 12992277]:
  _98 = (int) ufcMSR_52(D);
  k_105 = _98;
  _152 = MAX_EXPR <_98, 0>;
  i_49 = _152;

and all the i_49 are replaced with _152. 

However, the value range info for _152 doesnot reflect the one for i_49, it 
keeps as UNDEFINED. 

is this the root problem?  

thanks a lot.

Qing

> On Feb 28, 2019, at 1:54 PM, Jeff Law  wrote:
> 
> On 2/28/19 10:05 AM, Qing Zhao wrote:
>> Hi,
>> 
>> I have been debugging a runtime error caused by value range propagation. and 
>> finally located to the following gcc routine:
>> 
>> vrp_meet_1 in gcc/tree-vrp.c
>> 
>> 
>> /* Meet operation for value ranges.  Given two value ranges VR0 and
>>   VR1, store in VR0 a range that contains both VR0 and VR1.  This
>>   may not be the smallest possible such range.  */
>> 
>> static void 
>> vrp_meet_1 (value_range *vr0, const value_range *vr1)
>> {
>>  value_range saved;
>> 
>>  if (vr0->type == VR_UNDEFINED)
>>{
>>  set_value_range (vr0, vr1->type, vr1->min, vr1->max, vr1->equiv);
>>  return;
>>}
>> 
>>  if (vr1->type == VR_UNDEFINED)
>>{
>>  /* VR0 already has the resulting range.  */
>>  return;
>>}
>> 
>> 
>> In the above, when one of vr0 or vr1 is VR_UNDEFINED,  the meet result of 
>> these two will be  the other VALUE. 
>> 
>> This seems not correct to me. 
> That's the optimistic nature of VRP.  It's generally desired behavior.
> 
>> 
>> For example, the following is the located incorrect value range propagation: 
>>  (portion from the dump file *.181t.dom3)
>> 
>> 
>> Visiting PHI node: i_83 = PHI <_152(20), 0(22)>
>>Argument #0 (20 -> 10 executable)
>>_152: UNDEFINED
>>Argument #1 (22 -> 10 executable)
>>0: [0, 0]
>> Meeting
>>  UNDEFINED
>> and
>>  [0, 0]
>> to
>>  [0, 0]
>> Intersecting
>>  [0, 0]
>> and
>>  [0, 65535]
>> to
>>  [0, 0]
>> 
>> 
>> 
>> In the above, “i_83” is defined as PHI <_152(20), 0(22)>,   the 1st argument 
>> is UNDEFINED at this time(but its value range definitely is NOT [0,0]),
>> and the 2nd argument is 0.
> If it's value is undefined then it can be any value we want.  We choose
> to make it equal to the other argument.
> 
> If VRP later finds that _152 changes, then it will go back and
> reevaluate the PHI.  That's one of the basic design principles of the
> optimistic propagators.
> 
>> 
>> “vrp_meet” generate a VR_RANGE with [0,0] for “i_83” based on the current 
>> algorithm.  Obviously, this result VR_RANGE with [0,0] does NOT 
>> contain the value ranges for _152. 
>> 
>> the result of “vrp_meet” is Not correct.  and this incorrect value range 
>> result finally caused the runtime error. 
>> 
>> I ‘d like to modify the vrp_meet_1 as following:
>> 
>> 
>> static void 
>> vrp_meet_1 (value_range *vr0, const value_range *vr1)
>> {
>>  value_range saved;
>> 
>>  if (vr0->type == VR_UNDEFINED)
>>{
>>  /* VR0 already has the resulting range. */
>>  return;
>>}
>> 
>>  if (vr1->type == VR_UNDEFINED)
>>{
>>  set_value_range_to_undefined (vr0)
>> return;
>>}
>> 
>> 
>> let me know your opinion.
>> 
>> thanks a lot for the help.
> I think we (Richi and I) went through this about a year ago and the
> conclusion was we should be looking at how you're getting into the
> vrp_meet with the VR_UNDEFINED.
> 
> If it happens because the user's code has an undefined use, then, the
> consensus has been to go ahead and optimize it.
> 
> If it happens for any other reason, then it's likely a bug in GCC.  We
> had a couple of these when we made EVRP a re-usable module and started
> exploiting its data in the jump threader.
> 
> So you need to work backwards from this point to figure out how you got
> here.
> 
> jeff

Re: A bug in vrp_meet?

[Qing Zhao]

Jeff,

thanks a lot for the reply.

this is really helpful.

I double checked the dumped intermediate file for pass “dom3", and located the 
following for _152:

BEFORE the pass “dom3”, there is no _152, the corresponding Block looks 
like:

   [local count: 12992277]:
  _98 = (int) ufcMSR_52(D);
  k_105 = (sword) ufcMSR_52(D);
  i_49 = _98 > 0 ? k_105 : 0;

***During the pass “doms”,  _152 is generated as following:

Optimizing block #4
….
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

then bb 4 becomes:

   [local count: 12992277]:
  _98 = (int) ufcMSR_52(D);
  k_105 = _98;
  _152 = MAX_EXPR <_98, 0>;
  i_49 = _152;

and all the i_49 are replaced with _152. 

However, the value range info for _152 doesnot reflect the one for i_49, it 
keeps as UNDEFINED. 

is this the root problem?  

thanks a lot.

Qing

> On Feb 28, 2019, at 1:54 PM, Jeff Law  wrote:
> 
> On 2/28/19 10:05 AM, Qing Zhao wrote:
>> Hi,
>> 
>> I have been debugging a runtime error caused by value range propagation. and 
>> finally located to the following gcc routine:
>> 
>> vrp_meet_1 in gcc/tree-vrp.c
>> 
>> 
>> /* Meet operation for value ranges.  Given two value ranges VR0 and
>>   VR1, store in VR0 a range that contains both VR0 and VR1.  This
>>   may not be the smallest possible such range.  */
>> 
>> static void 
>> vrp_meet_1 (value_range *vr0, const value_range *vr1)
>> {
>>  value_range saved;
>> 
>>  if (vr0->type == VR_UNDEFINED)
>>{
>>  set_value_range (vr0, vr1->type, vr1->min, vr1->max, vr1->equiv);
>>  return;
>>}
>> 
>>  if (vr1->type == VR_UNDEFINED)
>>{
>>  /* VR0 already has the resulting range.  */
>>  return;
>>}
>> 
>> 
>> In the above, when one of vr0 or vr1 is VR_UNDEFINED,  the meet result of 
>> these two will be  the other VALUE. 
>> 
>> This seems not correct to me. 
> That's the optimistic nature of VRP.  It's generally desired behavior.
> 
>> 
>> For example, the following is the located incorrect value range propagation: 
>>  (portion from the dump file *.181t.dom3)
>> 
>> 
>> Visiting PHI node: i_83 = PHI <_152(20), 0(22)>
>>Argument #0 (20 -> 10 executable)
>>_152: UNDEFINED
>>Argument #1 (22 -> 10 executable)
>>0: [0, 0]
>> Meeting
>>  UNDEFINED
>> and
>>  [0, 0]
>> to
>>  [0, 0]
>> Intersecting
>>  [0, 0]
>> and
>>  [0, 65535]
>> to
>>  [0, 0]
>> 
>> 
>> 
>> In the above, “i_83” is defined as PHI <_152(20), 0(22)>,   the 1st argument 
>> is UNDEFINED at this time(but its value range definitely is NOT [0,0]),
>> and the 2nd argument is 0.
> If it's value is undefined then it can be any value we want.  We choose
> to make it equal to the other argument.
> 
> If VRP later finds that _152 changes, then it will go back and
> reevaluate the PHI.  That's one of the basic design principles of the
> optimistic propagators.
> 
>> 
>> “vrp_meet” generate a VR_RANGE with [0,0] for “i_83” based on the current 
>> algorithm.  Obviously, this result VR_RANGE with [0,0] does NOT 
>> contain the value ranges for _152. 
>> 
>> the result of “vrp_meet” is Not correct.  and this incorrect value range 
>> result finally caused the runtime error. 
>> 
>> I ‘d like to modify the vrp_meet_1 as following:
>> 
>> 
>> static void 
>> vrp_meet_1 (value_range *vr0, const value_range *vr1)
>> {
>>  value_range saved;
>> 
>>  if (vr0->type == VR_UNDEFINED)
>>{
>>  /* VR0 already has the resulting range. */
>>  return;
>>}
>> 
>>  if (vr1->type == VR_UNDEFINED)
>>{
>>  set_value_range_to_undefined (vr0)
>> return;
>>}
>> 
>> 
>> let me know your opinion.
>> 
>> thanks a lot for the help.
> I think we (Richi and I) went through this about a year ago and the
> conclusion was we should be looking at how you're getting into the
> vrp_meet with the VR_UNDEFINED.
> 
> If it happens because the user's code has an undefined use, then, the
> consensus has been to go ahead and optimize it.
> 
> If it happens for any other reason, then it's likely a bug in GCC.  We
> had a couple of these when we made EVRP a re-usable module and started
> exploiting its data in the jump threader.
> 
> So you need to work backwards from this point to figure out how you got
> here.
> 
> jeff

Re: A bug in vrp_meet?

[Jeff Law]

On 2/28/19 10:05 AM, Qing Zhao wrote:
> Hi,
> 
> I have been debugging a runtime error caused by value range propagation. and 
> finally located to the following gcc routine:
> 
> vrp_meet_1 in gcc/tree-vrp.c
> 
> 
> /* Meet operation for value ranges.  Given two value ranges VR0 and
>VR1, store in VR0 a range that contains both VR0 and VR1.  This
>may not be the smallest possible such range.  */
> 
> static void 
> vrp_meet_1 (value_range *vr0, const value_range *vr1)
> {
>   value_range saved;
> 
>   if (vr0->type == VR_UNDEFINED)
> {
>   set_value_range (vr0, vr1->type, vr1->min, vr1->max, vr1->equiv);
>   return;
> }
> 
>   if (vr1->type == VR_UNDEFINED)
> {
>   /* VR0 already has the resulting range.  */
>   return;
> }
> 
> 
> In the above, when one of vr0 or vr1 is VR_UNDEFINED,  the meet result of 
> these two will be  the other VALUE. 
> 
> This seems not correct to me. 
That's the optimistic nature of VRP.  It's generally desired behavior.

> 
> For example, the following is the located incorrect value range propagation:  
> (portion from the dump file *.181t.dom3)
> 
> 
> Visiting PHI node: i_83 = PHI <_152(20), 0(22)>
> Argument #0 (20 -> 10 executable)
> _152: UNDEFINED
> Argument #1 (22 -> 10 executable)
> 0: [0, 0]
> Meeting
>   UNDEFINED
> and
>   [0, 0]
> to
>   [0, 0]
> Intersecting
>   [0, 0]
> and
>   [0, 65535]
> to
>   [0, 0]
> 
> 
> 
> In the above, “i_83” is defined as PHI <_152(20), 0(22)>,   the 1st argument 
> is UNDEFINED at this time(but its value range definitely is NOT [0,0]),
>  and the 2nd argument is 0.
If it's value is undefined then it can be any value we want.  We choose
to make it equal to the other argument.

If VRP later finds that _152 changes, then it will go back and
reevaluate the PHI.  That's one of the basic design principles of the
optimistic propagators.

> 
> “vrp_meet” generate a VR_RANGE with [0,0] for “i_83” based on the current 
> algorithm.  Obviously, this result VR_RANGE with [0,0] does NOT 
> contain the value ranges for _152. 
> 
>  the result of “vrp_meet” is Not correct.  and this incorrect value range 
> result finally caused the runtime error. 
> 
> I ‘d like to modify the vrp_meet_1 as following:
> 
> 
> static void 
> vrp_meet_1 (value_range *vr0, const value_range *vr1)
> {
>   value_range saved;
> 
>   if (vr0->type == VR_UNDEFINED)
> {
>   /* VR0 already has the resulting range. */
>   return;
> }
> 
>   if (vr1->type == VR_UNDEFINED)
> {
>   set_value_range_to_undefined (vr0)
>  return;
> }
> 
> 
> let me know your opinion.
> 
> thanks a lot for the help.
I think we (Richi and I) went through this about a year ago and the
conclusion was we should be looking at how you're getting into the
vrp_meet with the VR_UNDEFINED.

If it happens because the user's code has an undefined use, then, the
consensus has been to go ahead and optimize it.

If it happens for any other reason, then it's likely a bug in GCC.  We
had a couple of these when we made EVRP a re-usable module and started
exploiting its data in the jump threader.

So you need to work backwards from this point to figure out how you got
here.

jeff

Re: A bug in vrp_meet?

[Jeff Law]

On 2/28/19 10:05 AM, Qing Zhao wrote:
> Hi,
> 
> I have been debugging a runtime error caused by value range propagation. and 
> finally located to the following gcc routine:
> 
> vrp_meet_1 in gcc/tree-vrp.c
> 
> 
> /* Meet operation for value ranges.  Given two value ranges VR0 and
>VR1, store in VR0 a range that contains both VR0 and VR1.  This
>may not be the smallest possible such range.  */
> 
> static void 
> vrp_meet_1 (value_range *vr0, const value_range *vr1)
> {
>   value_range saved;
> 
>   if (vr0->type == VR_UNDEFINED)
> {
>   set_value_range (vr0, vr1->type, vr1->min, vr1->max, vr1->equiv);
>   return;
> }
> 
>   if (vr1->type == VR_UNDEFINED)
> {
>   /* VR0 already has the resulting range.  */
>   return;
> }
> 
> 
> In the above, when one of vr0 or vr1 is VR_UNDEFINED,  the meet result of 
> these two will be  the other VALUE. 
> 
> This seems not correct to me. 
That's the optimistic nature of VRP.  It's generally desired behavior.

> 
> For example, the following is the located incorrect value range propagation:  
> (portion from the dump file *.181t.dom3)
> 
> 
> Visiting PHI node: i_83 = PHI <_152(20), 0(22)>
> Argument #0 (20 -> 10 executable)
> _152: UNDEFINED
> Argument #1 (22 -> 10 executable)
> 0: [0, 0]
> Meeting
>   UNDEFINED
> and
>   [0, 0]
> to
>   [0, 0]
> Intersecting
>   [0, 0]
> and
>   [0, 65535]
> to
>   [0, 0]
> 
> 
> 
> In the above, “i_83” is defined as PHI <_152(20), 0(22)>,   the 1st argument 
> is UNDEFINED at this time(but its value range definitely is NOT [0,0]),
>  and the 2nd argument is 0.
If it's value is undefined then it can be any value we want.  We choose
to make it equal to the other argument.

If VRP later finds that _152 changes, then it will go back and
reevaluate the PHI.  That's one of the basic design principles of the
optimistic propagators.

> 
> “vrp_meet” generate a VR_RANGE with [0,0] for “i_83” based on the current 
> algorithm.  Obviously, this result VR_RANGE with [0,0] does NOT 
> contain the value ranges for _152. 
> 
>  the result of “vrp_meet” is Not correct.  and this incorrect value range 
> result finally caused the runtime error. 
> 
> I ‘d like to modify the vrp_meet_1 as following:
> 
> 
> static void 
> vrp_meet_1 (value_range *vr0, const value_range *vr1)
> {
>   value_range saved;
> 
>   if (vr0->type == VR_UNDEFINED)
> {
>   /* VR0 already has the resulting range. */
>   return;
> }
> 
>   if (vr1->type == VR_UNDEFINED)
> {
>   set_value_range_to_undefined (vr0)
>  return;
> }
> 
> 
> let me know your opinion.
> 
> thanks a lot for the help.
I think we (Richi and I) went through this about a year ago and the
conclusion was we should be looking at how you're getting into the
vrp_meet with the VR_UNDEFINED.

If it happens because the user's code has an undefined use, then, the
consensus has been to go ahead and optimize it.

If it happens for any other reason, then it's likely a bug in GCC.  We
had a couple of these when we made EVRP a re-usable module and started
exploiting its data in the jump threader.

So you need to work backwards from this point to figure out how you got
here.

jeff

A bug in vrp_meet?

[Qing Zhao]

Hi,

I have been debugging a runtime error caused by value range propagation. and 
finally located to the following gcc routine:

vrp_meet_1 in gcc/tree-vrp.c


/* Meet operation for value ranges.  Given two value ranges VR0 and
   VR1, store in VR0 a range that contains both VR0 and VR1.  This
   may not be the smallest possible such range.  */

static void 
vrp_meet_1 (value_range *vr0, const value_range *vr1)
{
  value_range saved;

  if (vr0->type == VR_UNDEFINED)
{
  set_value_range (vr0, vr1->type, vr1->min, vr1->max, vr1->equiv);
  return;
}

  if (vr1->type == VR_UNDEFINED)
{
  /* VR0 already has the resulting range.  */
  return;
}


In the above, when one of vr0 or vr1 is VR_UNDEFINED,  the meet result of these 
two will be  the other VALUE. 

This seems not correct to me. 

For example, the following is the located incorrect value range propagation:  
(portion from the dump file *.181t.dom3)


Visiting PHI node: i_83 = PHI <_152(20), 0(22)>
Argument #0 (20 -> 10 executable)
_152: UNDEFINED
Argument #1 (22 -> 10 executable)
0: [0, 0]
Meeting
  UNDEFINED
and
  [0, 0]
to
  [0, 0]
Intersecting
  [0, 0]
and
  [0, 65535]
to
  [0, 0]



In the above, “i_83” is defined as PHI <_152(20), 0(22)>,   the 1st argument is 
UNDEFINED at this time(but its value range definitely is NOT [0,0]),
 and the 2nd argument is 0.

“vrp_meet” generate a VR_RANGE with [0,0] for “i_83” based on the current 
algorithm.  Obviously, this result VR_RANGE with [0,0] does NOT 
contain the value ranges for _152. 

 the result of “vrp_meet” is Not correct.  and this incorrect value range 
result finally caused the runtime error. 

I ‘d like to modify the vrp_meet_1 as following:


static void 
vrp_meet_1 (value_range *vr0, const value_range *vr1)
{
  value_range saved;

  if (vr0->type == VR_UNDEFINED)
{
  /* VR0 already has the resulting range. */
  return;
}

  if (vr1->type == VR_UNDEFINED)
{
  set_value_range_to_undefined (vr0)
 return;
}


let me know your opinion.

thanks a lot for the help.

Qing

A bug in vrp_meet?

[Qing Zhao]

Hi,

I have been debugging a runtime error caused by value range propagation. and 
finally located to the following gcc routine:

vrp_meet_1 in gcc/tree-vrp.c


/* Meet operation for value ranges.  Given two value ranges VR0 and
   VR1, store in VR0 a range that contains both VR0 and VR1.  This
   may not be the smallest possible such range.  */

static void 
vrp_meet_1 (value_range *vr0, const value_range *vr1)
{
  value_range saved;

  if (vr0->type == VR_UNDEFINED)
{
  set_value_range (vr0, vr1->type, vr1->min, vr1->max, vr1->equiv);
  return;
}

  if (vr1->type == VR_UNDEFINED)
{
  /* VR0 already has the resulting range.  */
  return;
}


In the above, when one of vr0 or vr1 is VR_UNDEFINED,  the meet result of these 
two will be  the other VALUE. 

This seems not correct to me. 

For example, the following is the located incorrect value range propagation:  
(portion from the dump file *.181t.dom3)


Visiting PHI node: i_83 = PHI <_152(20), 0(22)>
Argument #0 (20 -> 10 executable)
_152: UNDEFINED
Argument #1 (22 -> 10 executable)
0: [0, 0]
Meeting
  UNDEFINED
and
  [0, 0]
to
  [0, 0]
Intersecting
  [0, 0]
and
  [0, 65535]
to
  [0, 0]



In the above, “i_83” is defined as PHI <_152(20), 0(22)>,   the 1st argument is 
UNDEFINED at this time(but its value range definitely is NOT [0,0]),
 and the 2nd argument is 0.

“vrp_meet” generate a VR_RANGE with [0,0] for “i_83” based on the current 
algorithm.  Obviously, this result VR_RANGE with [0,0] does NOT 
contain the value ranges for _152. 

 the result of “vrp_meet” is Not correct.  and this incorrect value range 
result finally caused the runtime error. 

I ‘d like to modify the vrp_meet_1 as following:


static void 
vrp_meet_1 (value_range *vr0, const value_range *vr1)
{
  value_range saved;

  if (vr0->type == VR_UNDEFINED)
{
  /* VR0 already has the resulting range. */
  return;
}

  if (vr1->type == VR_UNDEFINED)
{
  set_value_range_to_undefined (vr0)
 return;
}


let me know your opinion.

thanks a lot for the help.

Qing