[llvm-commits] New classes for PR1255: Should enhance LLVM switch instruction to take case ranges.

[Stepan Dyatkovskiy]

Hi Chris,

In case if you want to implement APInt right now, I still propose to do it as a set of patches (step-by-step).
0. New classes that doesn't affect anything (I reattached it again). They still work with ConstantInt base values at this stage.
1. Fictitious replacement of current ConstantInt case values with case-ranges-set object (call it ConstantRangesSet by now, probably some better name exists). Case ranges set will still hold single value, and ConstantInt *getCaseValue() will return it. But additionally implement getCaseRangesSet() that will return new ConstantRangesSet object.
2. Step-by-step replacement of "ConstantInt* getCaseValue()" with "ConstantRangesSet getCaseRangesSet()". Modify algorithms for all passes that works with SwitchInst. But don't modify LLParser. Still hold just a single value in each ConstantRangesSet object. So on this stage some parts of LLVM will use old-style methods, and some ones new-style.
3. After all getCaseValue() usages will removed and whole LLVM and its clients will work in new style - modify LLParser and remove getCaseValue().
4. Replace ConstantInt*-based case ranges set items with APInt ones.

Relative to your sketch:
[quote]
SwitchInst's operands would *just* be the value being switched on and the BasicBlock destinations.

SwitchInst would contain an "std::vector<std::pair<unsigned,unsigned>> SuccessorIndices", whose length is the same as the # successors of the switch statement.

The pair for each destination is a range into another vector, "std::vector<APInt> CaseValues".

The mapping of the range to entries in CaseValues could look like this:

[N, N+1] -> Single element in CaseValues indicates the case value for the destination.
[MAXINT, MININT] -> The entire case is "default"
Otherwise, the pair indicates a series of ranges (inclusive).  If one of the ranges is MAXINT/MININT, then it is the default case.  For example [0, 4, 6,6]  would handle 0,1,2,3,4,6.  [4,6, MAXINT,MININT] would handle 4,5,6 and default.

A nice feature of this approach is that we would eliminate the requirement for a swtich to have a default case. Total switches (e.g. "switch (x&3)" with 4 cases) wouldn't need the extra range check at codegen time.
[/quote]
You propose to keep values in flat collection. What we gain in this case? Usually this approach used when values may be used by several objects, it allows to reduce the memory consumption. But values couldn't be reused, since it means that some ranges are intersected - we got undetermined behaviour in that case. I supporse that It also increases the access time.
If I get right...
For example we have two ranges
<0..5> -> Successor0
<6..7> -> Successor1
It will converted to the indices collection:
[0,5,6,7]
and successors vector (std::vector<std::pair<unsigned,unsigned>>):
[<0,1>,<2,3>] - 
And if I want to compute the range for Successor0. I need perform two actions:
1. int LowIdx = SuccessorRanges[0].first;
    int HighIdx = SuccessorRanges[0].second;
2. for (int i = LowIdx; i != HighIdx; ++i) { do something }

But what if we want to add some range to some existing case? We need to shake up whole successors indices, since they will changed.

I prefer your previous proposal: to keep set of ranges directly in the set of cases:

typedef std::pair<int, int> range_t;
typedef std::vector<range_t> case_t;
typedef std::vector<case_t> cases_t;
(It was here: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120102/134418.html)

-Stepan

14.02.2012, 15:39, "Stepan Dyatkovskiy" <STPWORLD at narod.ru>:
> Hi, Chris.
> You last thoughts relative to pr1255 was here:
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120102/134424.html
> As a reply to all previous discussion a created patch. I reattached it to this post (cr-0.7.17). It is just a two .h files with new classes. I tested it, but current patch adds new classes only and affects nothing.
>
>>  Please remind me why it makes sense to use ConstantArray (or something like it) here.  It seems that the case values of the switch could be very reasonably represented with APInts, eliminating the Value*'s for the case values all together (which is what I recommended in Comment 15 of PR1255).  OTOH, I do vaguely remember making the comment about arrays and vectors in some PR (but can't find it).  Thinking about this a third time now, I really think that avoiding Constant*'s is the right thing to do.
>
> What is the main reason to use APInts?
> I have some half-implemented version with APInts. IMHO, I looks not very good, since it goes agains LLVM architecture: SwitchInst will first instruction that contains some values that are not inherited from the Value. I was need to do a set of workarounds adapting LLVM infrastructure to this feature: back-ends, asm-writer, lazy-values, Verifier.cpp with its original assertions and so on. APInt numbers will like an aliens from another planet, since the will need to work with entities that are from LLVMContext, and often we need to move it back using ConstantInt::get(...). If you have a time you can look at second draft patch attached to this post (cr-0.5.0). I made it two months ago or so on. It also contains changes in indexing I already applied, filtering it you can see what will wait LLVM if we will use APInt for case values. Patch is really huge, just look how SimplifyCFG, AsmWriter and BitcodeWriter will changed.
> Anyway if there are some very reasonable args in favour of the APInt, I'll update all my changes respectively.
>
> -Stepan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cr-0.7.17-newclasses.patch
Type: application/octet-stream
Size: 18087 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120216/bb575451/attachment.obj>