[llvm-commits] Final SwitchInst for PR1255: case-ranges.

[Nuno Lopes]

Hi Stepan,

I took a quick look to your patch, and I've a few comments about it.
In general, I like the direction where it's going. It's much better  
than what it was before. But as I said in the previous email, I really  
would like to see some numbers about performance/memory regression.

Some comments about the patch itself:
  - I don't understand why you need the IntRange class. We already  
have ConstantRange, which does exactly the same thing, and it's even  
more memory efficient (it only stores 2 APInts; no extra bits needed).  
  The optimizations expect ConstantRanges as well.
  - I'm not sure you need IntegersSubset and IntegersSubsetGeneric.  
Can you explain why do have this extra layer?
  - I believe IntegersSubset should store ConstantRanges directly.  
It's as memory efficient as storing 2 APInts, and the code should  
become simpler.  I also missed the point of RangeLinks.
  - How is SwitchInst serialized to bitcode?  We cannot break the  
bitcode format until llvm 4.0, so we need a way to be binary  
compatible or provide an auto-upgrade path for older bit-code files.

I didn't look at IntegersSubsetMapping. Can you explain where and how  
it is used?  I think I remember some commits about that in CodeGen,  
but I don't remember the purpose.

Thanks,
Nuno


Quoting Stepan Dyatkovskiy <stpworld at narod.ru>:

> Hi all.
> Please find the patch that converts current SwitchInst into one that  
> supports case-ranges. This patch is not compilable, since I removed  
> all backward compatibility layer. This is how I see the final  
> SwitchInst.
>
> Main features
>  - Case value is the subset of integers, based on APInt values  
> (implemented as IntegersSubset). IntegersSubset uses packed data  
> representation. It has two collections: flat numbers collection, and  
> collection of pairs with indices (pointers) to the flat numbers  
> collection.
>  - To construct/modify subsets for SwitchInst IntegersSubsetMapping  
> was introduced. It uses unpacked data representation and stores  
> SwitchInst data as set of clusters. Cluster is integer range +  
> successor.
>  - Case value removed from instruction operands list. We needn't  
> User-Use concept here, since it is just a case value, it will always  
> constant, and case value couldn't reused with another  
> instructions/values.
>
> P.S.:  Patch contains a lot of changes. Probably you want to apply  
> it locally and look at final instruction class.
>
> P.S.2: I also have patch with backward compatibility level. It is  
> compilable and passes regression tests and test-suite tests.
>
> Thanks!
>
> -Stepan.