[llvm-commits] Please review the patch for IntegersSubsetMapping

[Stepan Dyatkovskiy]

 > I would like to see a discussion and review of the entire approach 
you are
taking.
I think it was finished here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120213/136954.html 

I selected approach proposed by Chris. Each case should be represented 
as set of ranges and numbers, or in another words it is called the 
subset of integers.

 > Maybe that's just because I don't understand them: they seem strangely
 > complicated, over-engineered, and I get lost in a mass of templates 
and other
 > abstractions.
I'm really trying to keep all terrible looking code deeply inside 
IntegersSubsetMapping.h and IntegersSubset.h. I need to introduce a lot 
of workarounds here due to double implementation: ConstantInt based and 
APInt based.
I try to make all "external" code as clean as it possible. Just look on 
passes already updated. I don't found they became more complex looking. 
LowerSwitch::Clusterify and SelectionDAGBuilder::Clusterify uses the 
single algorithm implemented in IntegersSubsetMapping, that was 
duplicated before. IMHO now these Clusterify methods looks even a little 
bit simpler.
Though I must add comments for "IntegersSubsetMapping::optimize" here. I 
separated TerminatorInst successors indexing from case indexing, so I 
think it also made llvm a little bit more clear looking.

I want to say more. I'm really worried about complexity of current code 
too. But I need to support old style switches that are ConstantInt 
based. And I need to move all passes to new style switches that are 
subsets based (and subsets are APInt based).
Let's consider templates I implemented.

- template <class IntTy> class IntegersSubsetGeneric
   I keep IntTy as template parameter since in llvm I need to use 
IntItem that is wrapper and contains double implementation (ConstantInt 
based, and it has APInt interface presented). In unittests I need to 
test generic algorithms and I can't use ConstantInt. So I can't use 
IntItem here. Yes. I can implement IntegersSubsetGeneric for APInts. But 
in that case we get tons of ConstaintInt::get() invocations and 
performance regressions. As additional present we got absolutely 
unreadable code: tons of ConstantInt::get and ConstantInt::getValue will 
replace simple references to the case items.
- IntegersSubset class - for me it is terrible to look at it. The 
purpose of this class is too maintain just-a-subset implementation and a 
Value-based implementation (ConstantArray of ConstantVectors of 
ConstantInts). All because some of passes wants to see Switch case value 
as Value that is linked with SwitchInst instance (ValueEnumerator, for 
example). Currently, the case value *must* be represented as Value.
- IntegersSubsetMapping. Currently it contains 3 params:
   -- SuccessorClass
   -- IntegersSubsetTy
   -- IntTy
   The two last params was introduced by the same reasons. In llvm 
itself I need wrappers with workarounds. In unittests I need generic 
classes. Wrapper params will be cleaned out after all passes will updated.

- SwitchInst::CaseIteratorT. I introduce this template to avoid code 
duplications for case iterator and const case iterator. Probably it is 
better just to implement both of them with code duplications. How do you 
think what is better (I'm really not sure here)?

I want to summarize this post with the next points:
1. All terrible code is localized in IntegersSubset.h and 
IntegersSubsetMapping.h.
2. Most of these code will be removed. I just need add proper 
case-ranges support for all passes, and then I can remove workarounds.