[patch] PR tree-optimization/96818 - cast label range to type of switch operand

Richard Biener richard.guenther@gmail.com
Mon Aug 31 12:55:19 GMT 2020


On Mon, Aug 31, 2020 at 11:10 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 8/31/20 10:33 AM, Richard Biener wrote:
> > On Mon, Aug 31, 2020 at 10:20 AM Aldy Hernandez <aldyh@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
>


More information about the Gcc-patches mailing list