[llvm-commits] Reworked SwitchInst prototype with case-ranges.

[Stepan Dyatkovskiy]

Hi Nuno! I'm glad to see this review and I hope that it is not the last 
one :-)

 > - I don't understand why do you need the SID. Why not use BasicBlock*
 > directly instead?  I don't understand why the need for this extra layer
 > of indirection.
I wrote the reason in original post. But, I'll try to give more details.

You need to keep the map "BB -> Range". Since ranges are represented as 
array, we got map "BB -> RangeIdx"
But this concept has the issue.
BB may be replaced during code transform. What to do with our map in 
this case? IMHO if we keep this map structure "as is", we need to do the 
next:
1. Handle BB replacement event with some way.
2. Go to our map and replace the BB was transformed,
I think that it is pretty complex to implement #1.
We can also store BB operand index instead. But here the same: if we 
remove some case (BB+ranges) we need to change part of remained BB 
indices in map. Though here we can just replace BB was removed with 
"unreachable" but it adds additional load for farther code transform passes.

 > - I guess the IRBuilder needs to have a nice interface to create
 > switches...
Did you mean some factory when user need to do something like this:
IRBuilder::SwitchBuilder sb;
sb.addCase(BB0, range0_0);
sb.addcase(BB0, range0_1);
...
sb.addcase(BB1, range1_0);
...
SwitchInst* SI = sb.constructSwitch();

It would be pretty nice concept to me.

Currently I built this approach into the SwitchInst itself, since it 
will keep without changing switch building. But as I said I also prefer 
the explicit factories.

3. Relative to defaults. IMHO default BB should be. It will less 
generic, but more comfortable for usage. We just add some cases and then 
say: "else do the default". And if user wishes to resolve the default BB 
he needn't to think "which BB is default here after all?", he just use 
SI->getDefaultDest().
I also think that we should skip default ranges during cases 
enumeration, since it may confuse the user: e.g. he adds 3 ranges, but 
got 5, since there is two default ranges between. To me it is just good 
to mark wasted ranges as defaults. In this case we needn't to rebuild 
the ranges array, and we keep it without holes.
So I propose to keep methods for default case "as is". So only 'switch' 
internals knows this secret :-)

The main question is the first issue (BB-to-ranges map). What to do if 
BB was replaced/removed? I see two real approaches:
1. SIDs.
2. Keeping BB operand indices instead of BBs itself. If some case was 
removed, insert 'unreacheble' BB instead to keep all remained operand 
indices the same.
3. Any ideas?

-Stepan.