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

[Chris Lattner]

On Feb 13, 2012, at 10:19 PM, Stepan Dyatkovskiy wrote:

> Hi all!
> So, what about new classes? How you find it? Any comments?

Hi Stephan,

I'm not sure what you're asking here and a lot of this is paged out of my brain at this point, please remind me :).  Also, please attach the patch that you want to be considered.

>>>>   11.02.2012, 13:52, "Stepan Dyatkovskiy"<STPWORLD at narod.ru>:
>>>>>   Hi all. I just finished new classes for PR1255.
>>>>> 
>>>>>   NEW CLASS.
>>>>>   As was proposed by Chris in recent PR1255 related posts, I selected ConstantArrays as case ranges set "holder" object. The array items are may be ConstantVectors with single item, and ConstantVectors with two items (that means single number and range respectively). Though I think that it would be better to hide the parsing of ConstantArray and create new class that will use ConstantArray inside.

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.

We can discuss different data structures to represent the important common cases, but here is a simple non-optimized strawman sketch to get things going:

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.

As an invariant, we could keep the entries sorted.

>>>>>   New class should contain two features:
>>>>>   - bool isSatisfies(ConstantInt *V) method (need better name?). Returns "true" if the given value satisfies this case.

Given a datastructure like the above, we would definitely want helper methods on SwitchInst to expose this information.  We'd want the existing getDefaultCase() (which could now return null) and findCaseValue() methods.  We would also want a getValuesForSuccessor() method that would return the APInt list for a given successor (abstracting out the first vector).

>>>>>   At first I tried to use ConstatArray as a parent for new class. But ConstantArray::get(...) method may produce zeroinitalizers and data aggregators.

This is one reason among many that you don't want to use Constant*'s to represent this.

-Chris