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

Aldy Hernandez aldyh@redhat.com
Mon Aug 31 09:10:25 GMT 2020



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.

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?

> 
> 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