[llvm-commits] [llvm] r159527 - in /llvm/trunk: include/llvm/Support/IntegersSubset.h include/llvm/Support/IntegersSubsetMapping.h lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp lib/Transforms/Utils/LowerSwitch.cpp lib/Transforms/Utils/Simplify

[Stepan Dyatkovskiy]

Hi Chandler,

 >> - Changed isSingleNumber method behaviour. Now this flag is 
calculated on demand.
 > Why?
Before patch this value was calculated each time in constructor
  IntRange(const IntType &L, const IntType &H) : Low(L), High(H),
       IsEmpty(false), IsSingleNumber(L == H) {}
Actually isSingleNumber check is needed not for all Range objects. If we 
will calculate L == H on demand (that is actually APInt::eq) will 
performed for objects it need only. Note, once it was calculated for 
object it will stored in RangeType field. Then isSingleNumber returns 
the RangeType == SINGLE_NUMBER.

 >     IntegersSubsetMapping
 >        - Optimized diff operation.
 >
 >
 > Under what cases? How much? Did you check in a test case to catch
 > regressions?
I optimized it for cases when subset contains single numbers only (not 
ranges). Generic algorithms that represents each item as range works 
slowly for this case. Without this optimization after updating 
SimplifyCFG (add support for case-ranges) I got 15% regression summary. 
On some tests LLC shows 50% regression and more.

 >        - Replaced type of Items field from std::list with std::map.
 >
 >
 > Why? This type of datastructure change is fairly surprising. We usually
 > move *away* from std::map, not toward it.
I need to keep IntegersSubsetMapping::Items collection sorted always, 
since almost all operations on mapping need it. Keeping Items ordered 
helps to perform IntegersSubsetMapping::isOverlapped and 
IntegersSubsetMapping::verify methods for example. It also helps to 
calculate difference between two subsets and so on.
Though I think it is possible to replace it with DenseMap.

 >        - Added new methods:
 >          bool isOverlapped(self &RHS)
 >          void add(self& RHS, SuccessorClass *S)
 >          void detachCase(self& NewMapping, SuccessorClass *Succ)
 >          void removeCase(SuccessorClass *Succ)
 >          SuccessorClass *findSuccessor(const IntTy& Val)
 >          const IntTy* getCaseSingleNumber(SuccessorClass *Succ)
 >
 >
 > But you didn't add any new tests for them. That's unacceptable.

Sorry. I'll add them.

P.S.: Warnin was fixed in r159532.

-Stepan.

Chandler Carruth wrote:
> No, read my comments, and respond to them.
>
> Also, fix the warnings you introduced.
>
>
> On Mon, Jul 2, 2012 at 6:21 AM, Stepan Dyatkovskiy <stpworld at narod.ru
> <mailto:stpworld at narod.ru>> wrote:
>
>     Chandler Carruth wrote:
>
>         I haven't had time to review this in detail, however, I have a
>         strong
>         meta-level comment: please break these patches up before you
>         submit them!
>
>     OK. Should I reject it now?
>
>