[llvm-commits] Reworked SwitchInst prototype with case-ranges.

[Nuno Lopes]

Hi Stepan,

Sorry for the delay. I was off-line on vacations.

Some comments:
 - I don't understand why do you need the SID. Why not use BasicBlock* 
directly instead?  I don't understand why the need for this extra layer of 
indirection.

 - still regarding the SIDs, to support removing ranges I think we only need 
2 operations: remove a BB (because it's unreachable), and therefore merge 
with nearby ranges; and remove an interval that is not reachable  (which can 
probably be done nicely from an iterator).

 - I don't think we should have UnsortedRanges nor all those DenseMaps. 
It's duplicating the memory requirements for switch statements for no good 
reason, I guess.

 - Does the default BB exist or not?  I think we agreeded we didn't need 
that, since holes are not allowed with this representation, and the last 
(possibly wrapping) range will take care of the default case.

 - I guess the IRBuilder needs to have a nice interface to create switches. 
Otherwise it will be very painful for frontends to do it by "hand".  Also, 
if not done carefully, I can imagine the whole process being O(n^2), since 
adding n switch cases may require traversing all the other intervals to 
lookup the place to insert it into.
And by this I mean that probably the clustering can be extracted to 
IRBuilder. We shouldn't store temporary data that is only used for the 
construction of the switch statement in the statement itself. The IRBuilder 
can be used for that stuff and then be disposed.

 - Please don't use function static variables. It's not good for thread 
safety. And we certainly don't need a global switch case numbering.


To conclude, I like the direction where this is going, but I still would 
like to see the points mentioned above polished before the patch goes in.

Thanks,
Nuno


----- Original Message -----
> ping
> Stepan Dyatkovskiy wrote:
>> Hi all, please find the new SwitchInst prototype with case ranges in
>> attachment.
>>
>> Main features.
>>
>> 1. Ranges are stored in flat ordered numbers collection (thanks to
>> Duncan and Chandler for this idea). Range [Low, High) may be represented
>> as Low=collection[i], High=collection[i+1]. Thus all set of integers is
>> covered. The collection is terminated by the wrapped range:
>>    Low=collection[collection.size()-1], High=collection[0].
>> For each collection item we also assign BasicBlock, Range+BasicBlock
>> will called "Cluster". E.g.:
>>    [1 -> Default] [2 -> Successor0] [5 -> Successor1] [8 -> Successor2]
>>    That means that we have 3 clusters:
>>    [2, 5) -> Successor0
>>    [5, 8) -> Successor1
>>    [8, 1) -> Successor2 // This range is wrapped.
>>
>> 2. I introduced successor ID (SID).
>> We need associate successors with case-ranges. But both ranges and
>> successors collections may be changed asynchronously:
>>    Successors may change its indices during 'remove' operation.
>>    Ranges may be removed.
>> But we need to keep always actual successor information for each range.
>> Even if successor was replaced by optimizer or its index was changed.
>> That's why SID was introduced, it will the same always. In that case
>> several SIDs may be assigned to the same successor after optimization.
>>
>> 3. Ranges directed to the default successor are always skipped during
>> iteration. Thus when user invokes getNumRanges, he got number of
>> non-default ranges. When user gets iterator case_begin(), he got
>> iterator to the first non-default range. And so on.
>> Probably its is less generic way. But it helps to reduce ranges
>> processing time. Since we process default after all, as some kind of
>> remainder.
>>
>> 4. Support for clusterification. Iterator has method
>> getClusterAndGoNext. This method allows to iterate over merged ranges
>> with the same successor and neighbouring borders. So if you need to
>> collect optimized clusters you just should use
>> CaseIt::getClusterAndGoNext instead of standard ++ operation.
>>
>> P.S.: IntegersSubset, IntegersSubsetMapping and IntegersSubsetTest will
>> go away. I excluded these changes from current patch for better and
>> cleaner looking.
>>
>> Thanks,
>> -Stepan.