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

[Richard Biener]

On September 3, 2020 5:39:18 PM GMT+02:00, Andrew MacLeod <amacleod@redhat.com> wrote:
>On 9/3/20 3:43 AM, Richard Biener wrote:
>> On Thu, Sep 3, 2020 at 9:21 AM Aldy Hernandez <aldyh@redhat.com>
>wrote:
>>>
>>>
>>> On 8/31/20 2:55 PM, Richard Biener wrote:
>>>> 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 :/
>>> 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>).
>typedef int_range<3> irange3;
>
>isnt a real thing.. its a typedef Aldy used in the range-ops self tests
>
>as shorthand.. I think its a leftover from a previous incarnation of 
>irange.
>
>>
>> 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?
>
>irange is the generic class that is used to operate on integral
>ranges.  
>It implements all the range operations and provides the API. it is 
>agnostic to the number of sub ranges.
>
>int_range is the template that instantiates an irange object, and 
>associates enough memory to represent the number of sub-ranges you want
>
>to work with.. so it is a bit more than a typedef, its a derived class 
>which adds memory.
>
>declaring a function
>foo (int_range<3> &result, op1, op2)
>
>wont allow you to pass an int_range<4> as the argument... it will take 
>nothing but an int_range<3> as they are different types in c++... which
>
>is very limiting
>
>foo (irange &result, op1, op2)
>
>on the other hand allows you to pass any implementation/instantiation 
>around you want. So we use irange everywhere we want to work with an 
>generic integral range, and when you need to declare a variable to
>store 
>a range, you use int_range<x>
>
>
>>
>> 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?
>>
>Indeed, I had forgotten about default template arguments, the only 
>problem is
>
>int_range x;
>
>is valid in c++17, but previous to that we have to do
>
>int_range<> x;
>
>Its a bit ugly, and possibly a bit less natural, But it does show the 
>intent.
>
>So I think the 2 basic choices are:
>
>int_range<> r;
>int_range_max r;
>
>I'm thinking 'int_range_max'?    but I really don't feel strongly one 
>way or the other...   Eventually we'll probably end up with c++17 and
>we 
>can revert to  'int_range' everywhere.
>If we go with the _max suffix, then we should stick to the "int_range" 
>pattern for consistency.. in hindsight, 'widest_irange' should have
>been 
>'widest_int_range'.

Int_range_max works for me. 

Richard. 

>Andrew
>
>There is also a pool allocator coming which will give you a dynamically
>
>allocated irange object with just enough memory associated to represent
>
>a specified range object.
>This will allow you to calculate a range using maximal sub-ranges, but 
>then store the result away efficeiently using just the required number 
>of sub-ranges.