[llvm-commits] [Patch] Replace switches with lookup tables (PR884)

[Sean Silva]

>> In the switch lowering code, there is code that does this kind of
>> analysis (e.g., breaking the cases into groups). I'm not sure how
>> easy/hard it would be to reuse though.
>
> RIght. I think it should be done in a separate patch, though.

For sure.

> You got me :) No, I did not run experiments. I did peek at the code in
> SelectionDAGBuilder.cpp that builds the jump tables, and that has the
> same check (line 2095). I can put in a note about that, or a fixme to
> figure out the best cutoff.

Yes, a cross reference would be good here, along with the FIXME :)

>> +  if (SI->getNumCases() * 10 < TableSize * 4) {
>> +    TableTooSparse = true;
>>
>
> This one is also to match the jump table logic (SelectionDAGBuilder.cpp:2104).

Same for this one. Cross reference + FIXME.

+  // Chicken out for large spreads to avoid any overflows below.
+  if (RangeSpread.zextOrSelf(64).ugt(UINT64_MAX / 10))
+    return false;

On my initial review, I honestly didn't see the connection between
this and the *10 a couple lines down (I guess I interpreted "below" as
a bit farther down). You should factor this and the surrounding code
into a static function isTooSparse(), and do like:

if (isTooSparse(...))
  return false;

+  // FIXME: Handle unreachable cases.
[...]
+  // Check whether the condition value is within the case range, and branch.
+  Builder.SetInsertPoint(SI);
+  Value *IsAboveMin = Builder.CreateICmpSGE(SI->getCondition(), MinCaseVal);
+  Value *IsBelowMax = Builder.CreateICmpSLE(SI->getCondition(), MaxCaseVal);
+  Value *IsBoth = Builder.CreateAnd(IsAboveMin, IsBelowMax);
+  Builder.CreateCondBr(IsBoth, LookupBB, SI->getDefaultDest());

I'm guessing that in a lot of the situations where this lookup table
optimization would kick in the program has undefined behavior if the
value is out of range, so this check could be elided. I think that
handling unreachable cases is a good direction for a future patch :)

--Sean Silva
On Mon, Sep 3, 2012 at 12:42 PM, Hans Wennborg <hans at chromium.org> wrote:
> Thank you very much for the review! New patch attached.
>
> On Mon, Sep 3, 2012 at 4:16 PM, Sean Silva <silvas at purdue.edu> wrote:
>> I really like the idea of this patch, and I think it can have a
>> significant code-size improvement. One huge improvement I expect is
>> for constructs like
>>
>> const char *getStringOf(enum Foo val) {
>> switch(val) {
>> case Foo_Bar: return "Bar";
>> case Foo_Baz: return "Baz";
>> ...
>> }
>>
>> not that that code is in the hot patch (I hope).
>
> Yes, that's the kind of code I'm going after. And we have quite a bit
> of that in LLVM too (for example, lib/Support/Dwarf.cpp). There are
> also lots of functions like this that just return true or false, e.g.
> the functions in
> http://clang.llvm.org/doxygen/PrintfFormatString_8cpp_source.html#l00568
> And LLVM is pretty good in synthesizing switches from if statements,
> such as Example 7 on http://blog.regehr.org/archives/767, where I
> think the shift/mask optimization comes is nicely.
>
>> +  // Maybe we can squeeze the table into an integer.
>>
>> The small integer shift/mask optimization seems like it could be a
>> separate patch. The FIXME for using the smallest integer type possible
>> to pack the bitmask seems like it should be an integral part of the
>> logic, rather than an afterthought, and doing the small integer
>> optimization there would allow for a solid focused implementation.
>> Also, the code for handling bitmap lookup tables is really
>> intermingled with the regular lookup table code; it seems like this
>> could be factored better, and a separate patch would naturally
>> encourage that.
>
> I agree, I'll remove this from the current patch.
>
>> +  // FIXME: If the switch is too dense for a lookup table, perhaps we could
>> +  // split off a dense part and build a lookup table for that.
>>
>> Do you mean "if the switch is too sparse"?
>
> Yup, fixed.
>
>> In the switch lowering code, there is code that does this kind of
>> analysis (e.g., breaking the cases into groups). I'm not sure how
>> easy/hard it would be to reuse though.
>
> RIght. I think it should be done in a separate patch, though.
>
>>
>> +  // If the number of cases is too small, don't bother.
>> +  if (SI->getNumCases() < 4)
>> +    return false;
>>
>> eww.... hard coded constant threshold. If this was chosen
>> experimentally, then I applaud you, but my guess is that it wasn't.
>> Some details of the experimental setup that chose this would be good
>> in the former case, and a FIXME begging to run some experiments to
>> find a good value for this would be good in the latter case.
>
> You got me :) No, I did not run experiments. I did peek at the code in
> SelectionDAGBuilder.cpp that builds the jump tables, and that has the
> same check (line 2095). I can put in a note about that, or a fixme to
> figure out the best cutoff.
>
>> +  // Chicken out for large spreads to avoid any overflows below.
>> +  if (RangeSpread.zextOrSelf(64).ugt(1 << 20))
>> +    return false;
>
> Yeah, this is arbitrary. Checking that it's <= UINT64_MAX / 10 should
> be sufficient. I'll update the patch to do that.
>
>>
>> +  if (SI->getNumCases() * 10 < TableSize * 4) {
>> +    TableTooSparse = true;
>>
>
> This one is also to match the jump table logic (SelectionDAGBuilder.cpp:2104).