On Thu, Sep 3, 2020 at 9:21 AM Aldy Hernandez <al...@redhat.com> wrote: > > > > On 8/31/20 2:55 PM, Richard Biener wrote: > > On Mon, Aug 31, 2020 at 11:10 AM Aldy Hernandez <al...@redhat.com> wrote: > >> > >> > >> > >> On 8/31/20 10:33 AM, Richard Biener wrote: > >>> On Mon, Aug 31, 2020 at 10:20 AM Aldy Hernandez <al...@redhat.com> 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 >