This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] More value_range API cleanup


On 11/12/18 7:12 AM, Richard Biener wrote:

This mainly tries to rectify the workaround I put in place for ipa-cp.c
needing to build value_range instead of value_range_base for calling
extract_range_from_unary_expr.

To make this easier I moved more set_* functions to methods.

Then for some reason I chose to fix the rathole of equiv bitmap sharing
after finding at least one real bug

By the way, I've seen that the equiv_add() calls in vr-values.c sometimes set equivalences for VARYING and UNDEFINED which in theory shouldn't happen. I've been too chicken to follow that hole.

I think we should assert everywhere that we set the equivalences, that we're not talking about VARYING or UNDEFINED.

@@ -6168,37 +6172,30 @@ value_range::union_helper (value_range *vr0, const value_range *vr1)
        return;
      }
- value_range saved (*vr0);
    value_range_kind vr0type = vr0->kind ();
    tree vr0min = vr0->min ();
    tree vr0max = vr0->max ();
    union_ranges (&vr0type, &vr0min, &vr0max,
  		vr1->kind (), vr1->min (), vr1->max ());
-  *vr0 = value_range (vr0type, vr0min, vr0max);
-  if (vr0->varying_p ())
+  /* Work on a temporary so we can still use vr0 when union returns varying.  */
+  value_range tem;
+  tem.set_and_canonicalize (vr0type, vr0min, vr0max);
+  if (tem.varying_p ())

I'm not a big fan of the code duplication in the union chunks. You're adding more places that need to be kept in sync.

I think value_range::union_ could be easily coded as:

  value_range_base::union_ (other);
  union_helper (this, other);
  if (flag_checking)
    check ();

And have union_helper only deal with the equivalence stuff. Call it union_equivs? You'd have to clear the equivalences if the range just became a varying/undefined, as both of those should in theory never have equivalences.

Also, is there a reason why you implemented value_range_base::union_ but not the corresponding for intersect? I would guess it'd be needed sooner or later.

Thanks for working on this.
Aldy


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]