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

Jason Merrill jason@redhat.com
Mon Aug 31 21:20:05 GMT 2020


On 8/31/20 5:10 AM, Aldy Hernandez 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.
> 
> 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.

I don't remember any such decision.  In C++, the case values are 
converted to the type of the switch argument (in case_conversion).

Jason



More information about the Gcc-patches mailing list