On Thu, Sep 3, 2020 at 9:21 AM Aldy Hernandez <[email protected]> wrote:
>
>
>
> On 8/31/20 2:55 PM, Richard Biener wrote:
> > On Mon, Aug 31, 2020 at 11:10 AM Aldy Hernandez <[email protected]> wrote:
> >>
> >>
> >>
> >> On 8/31/20 10:33 AM, Richard Biener wrote:
> >>> On Mon, Aug 31, 2020 at 10:20 AM Aldy Hernandez <[email protected]> wrote:
> >>>>
> >>>> As discussed in the PR, the type of a switch operand may be different
> >>>> than the type of the individual cases. This is causing us to attempt to
> >>>> intersect incompatible ranges.
> >>>>
> >>>> This inconsistent IL seems wrong to me, but it also seems pervasive
> >>>> throughout GCC. Jason, as one of the original gimple designers, do you
> >>>> remember if this was an oversight, or was there a reason for this?
> >>>>
> >>>> Richard, you mention in the PR that normalizing the IL would cause
> >>>> propagation of widening cast sources into switches harder. Couldn't we
> >>>> just cast all labels to the switch operand type upon creation? What
> >>>> would be the problem with that? Would we generate sub-optimal code?
> >>>>
> >>>> In either case, I'd hate to leave trunk in a broken case while this is
> >>>> being discussed. The attached patch fixes the PRs.
> >>>>
> >>>> OK?
> >>>
> >>> widest_irange label_range (CASE_LOW (min_label), case_high);
> >>> + if (!types_compatible_p (label_range.type (), range_of_op->type
> >>> ()))
> >>> + range_cast (label_range, range_of_op->type ());
> >>> label_range.intersect (range_of_op);
> >>>
> >>> so label_range is of 'widest_int' type but then instead of casting
> >>> range_of_op to widest_irange you cast that widest_irange to the
> >>> type of the range op? Why not build label_range in the type
> >>> of range_op immediately?
> >>
> >> The "widest" in widest_irange does not denote the type of the range, but
> >> the number of sub-ranges that can maximally fit. It has nothing to do
> >> with the underlying type of its elements. Instead, it is supposed to
> >> indicate that label_range can fit an infinite amount of sub-ranges. In
> >> practice this is 255 sub-ranges, but we can adjust that if needed.
> >
> > That's definitely confusing naming then :/
>
> I've been mulling this over, and you're right. The naming does imply
> some relation to widest_int.
>
> Originally I batted some ideas on naming this, but came up short. I'm
> still not sure what it should be.
>
> max_irange?
>
> biggest_irange?
>
> Perhaps some name involving "int_range", since int_range<> is how ranges
> are declared (int_range_max??).
The issue is that widest_irange has a connection to the type of the range
while it seems to constrain the number of subranges. So if there's
a irange<1>, irange<2>, etc. then irange_max would be a typedef that
makes most sense (both <2> and _max are then suffixes).
Hmm, a simple grep reveals
value-range.h:typedef int_range<255> widest_irange;
value-range.h:class GTY((user)) int_range : public irange
so widest_irange is not an irange but an int_range? But then
there's little use of 'irange' or 'int_range', most code uses
either irange & arguments (possibly accepting sth else than
int_range) and variables are widest_irange or irange3
(typedefed from int_range<3>).
IMHO a typedef irange* to int_range is confusing a bit.
Why isn't it int_range3 or widest_int_rage? Why is there
irange and int_range?
Well, still stand with irange_max, or, preferably int_range_max.
Or ...
template<unsigned N = 255>
class GTY((user)) int_range : public irange
{
so people can use
int_range x;
and get the max range by default?
But back to the question of just renaming widest_irange. Use irange_max.
The other confusion still stands of course.
Richard.
> Suggestions welcome.
> Aldy
>
