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

[Nuno Lopes]

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

/me too.


> > - 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"

Uhm, I see. This map can be used when performing backwards range analysis 
(e.g., LazyValueInfo - LVI). Right now LVI has to iterate over all switch 
cases.
For example, can we have this map being built lazily, and then cached? If 
something changes in the switch, discard the cache.
I'm just trying to reduce the memory usage, since I'm not sure if this map 
will be used that often.


> > - 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.

Yes, I like something like that. and a 'addDefault(range)' as well, so that 
the frontend doesn't need to fill in the holes.


> 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 :-)

But with a proper factory in place the user doesn't need to care about 
filling holes, so I think the default case should go away.

Nuno