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 :/
> So, in the constructor:
>
> widest_irange label_range (CASE_LOW (min_label), case_high);
>
> ...the type for the sub-ranges in label_range is the type of
> CASE_LOW(min_label) and case_high. They must match IIRC.
>
> I am working on an irange best practices document that should go into
> detail on all this, and will post it later this week.
>
> >
> > Using widest_int for both would have been obvious to me, you're
> > indicating that switch (type op) { case type2 lab: } is supposed to
> > match as (type)lab == op rather than (type2)op == lab. I think
> > the IL will be consistent in a way that sign-extending both
> > op and lab to a common larger integer type is correct
> > (thus using widest_int), but no truncation should happen here
> > (such trucation should be applied by the frontend).
>
> I suppose we could convert both label_range and range_of_op to a wider
> type and do the intersection on these results. Is there a tree type
> that indicates a widest possible int?
Btw, just looked up how we expand GIMPLE_SWITCH and there we
convert case label values to the type of the index expression
(what your patch did).
So the patch is OK.
Richard.
> >
> > In any case type mismatches here are of course unfortunate
> > and both more verification and documentation would be
> > nice. verify_gimple_switch only verifies all case labels
> > have the same type, the type of the switch argument is
> > not verified in any way against that type.
>
> Is the verification good enough with what Jakub posted? I can adjust
> the documentation, but first I'd like a nod from Jason that this was
> indeed expected behavior.
>
> Thanks.
> Aldy
>
