Switch containing holes via table lookup

[Hans Wennborg]

On Wed, Feb 12, 2014 at 3:40 PM, Jasper Neumann <jn at sirrida.de> wrote:
> But, as I wrote, my first attempt almost but unfortunately not always works.
> When I try to compile LLVM with modified compiler it crashes at
> lib/AsmParser/LLParser.cpp after successfully converting several switches to
> table lookups from other files.
> The error is as follows:
> clang: /home/jane/wrk/llvm-trunk/include/llvm/IR/Instructions.h:2155:
> llvm::Value* llvm::PHINode::getIncomingValueForBlock(const
> llvm::BasicBlock*) const: Assertion `Idx >= 0 && "Invalid basic block
> argument!"' failed.
> [...]
> The statistics I got thus far tells me that those fixable holes occur quite
> often.

Cool!

> I probably have missed to insert a connection between BasicBlocks.
> There should be a way to apply the methods described in
> http://llvm.cs.uiuc.edu/releases/2.3/docs/tutorial/LangImpl5.html.
> Please help to fix this.

I haven't tried to debug your patch, but I've taken a closer look and
made some comments. Hopefully we can make the code a little more
straight-forward and then figure out where the bug is.

> +static cl::opt<bool> CheckHoles(
> +    "simplifycfg-check-holes", cl::Hidden, cl::init(true),
> +    cl::desc("Allow holes in a value table by testing a mask"));

Having this command-line option is just for testing, right?

> +  unsigned NeededMaskSize = 0;
> +  if (CheckHoles) {
> +    if (TableSize <= 32 && TD->fitsInLegalInteger(32))
> +      NeededMaskSize = 32;
> +    else if (TableSize <= 64 && TD->fitsInLegalInteger(64))
> +      NeededMaskSize = 64;

What if the table is of size 12? We don't want to have to use a 32-bit
immediate for the mask in that case. I think we should generate the
bitmask of the same size as the table, as long as it fits in a legal
register. This is what the current bitmap-building code for lookup
tables does (see the SwitchLookupTable constructor above).

> +  // If the table has holes, we need a constant result for the default case...
>    SmallVector<std::pair<PHINode*, Constant*>, 4> DefaultResultsList;
> -  if (TableHasHoles && !GetCaseResults(SI, 0, SI->getDefaultDest(), &CommonDest,
> -                                       DefaultResultsList, TD))
> +  bool NeedMask = (TableHasHoles &&
> +                  !GetCaseResults(SI, 0, SI->getDefaultDest(), &CommonDest,
> +                                  DefaultResultsList, TD));
> +  // ...or need to check for valid values with a sufficiently small bit set.
> +  if (NeedMask && NeededMaskSize == 0)
>      return false;

I wonder if we could simplify the code here. What if we start with

bool HasDefaultResults = false;
if (TableHasHoles)
  // If the table has holes, check if we can fill them with a default result.
  HasDefaultResults = GetCaseResults(SI, 0, SI->getDefaultDest(),
&CommonDest, DefaultResultsList, TD);

ConstantInt *TableMask = 0;
if (TableHasHoles && !HasDefaultResults && TD->fitsInLegalInteger(TableSize)) {
  // Build a bitmask to check for valid table indices.
  APInt MaskInt(TableSize, 0);
  APInt One(TableSize, 1);
  const ResultListTy &ResultList = ResultLists[PHIs[0]];
  for (size_t I = 0, E = ResultList.size(); I != E; ++I) {
    uint64_t Idx = (ResultList[I].first->getValue() -
MinCaseVal->getValue()).getLimitedValue();
    MaskInt |= One << Idx;
  }
  TableMask = ConstantInt::get(M.getContext(), MaskInt);
}


Now we can take all these things together and decide if we want to
bail out or not:

if (TableHasHoles && !HasDefaultResults && !TableMask)
  return false;

> +    // As an extra penalty for the validity test we require more cases.
> +    if (SI->getNumCases() < 4)  // TODO: Make threshold configurable.
> +      return false;

Hmm, yeah I guess this extra penalty might be necessary :/

> +    // We need any value to fill the table; the first one suffices.
> +    SwitchInst::CaseIt CI = SI->case_begin();
> +    GetCaseResults(SI, CI.getCaseValue(), CI.getCaseSuccessor(),
> +                   &CommonDest, DefaultResultsList, TD);

Ouch, calling GetCaseResults more than necessary is something I'd like
to avoid. We already have all the results in ResultLists, so let's use
that.

>    // Populate the BB that does the lookups.
>    Builder.SetInsertPoint(LookupBB);
> +
> +  if (NeedMask) {
> +    uint64_t UseMask = 0;
> +    for (SwitchInst::CaseIt CI = SI->case_begin(), E = SI->case_end();
> +         CI != E; ++CI) {
> +      ConstantInt *CaseVal = CI.getCaseValue();
> +      UseMask |= 1ULL <<
> +        (CaseVal->getValue() - MinCaseVal->getValue()).getLimitedValue();
> +    }
> +
> +    BasicBlock *MaskBB = BasicBlock::Create(Mod.getContext(),
> +                                            "switch.lookup2",
> +                                            CommonDest->getParent(),
> +                                            CommonDest);

I think we should create this BB earlier (and with a better name!), so
that we can use it in the conditional branch we create after checking
that Index < TableSize above. I also like to have the mask built
before we start generating IR.

> +    ConstantInt *Zero;
> +    ConstantInt *One;
> +    ConstantInt *Mask;
> +    Value *CmpVal;
> +    if (NeededMaskSize == 32) {
> +      Zero = Builder.getInt32(0);
> +      One = Builder.getInt32(1);
> +      Mask = Builder.getInt32(UseMask);
> +      CmpVal = Builder.CreateZExtOrTrunc(TableIndex, Builder.getInt32Ty());
> +    } else {
> +      Zero = Builder.getInt64(0);
> +      One = Builder.getInt64(1);
> +      Mask = Builder.getInt64(UseMask);
> +      CmpVal = Builder.CreateZExtOrTrunc(TableIndex, Builder.getInt64Ty());
> +    }
> +    Value *Shr = Builder.CreateLShr(Mask, CmpVal);
> +    Value *And = Builder.CreateAnd(Shr, One);
> +    Value *CmpZ = Builder.CreateICmpNE(And, Zero);
> +    Builder.CreateCondBr(CmpZ, MaskBB, SI->getDefaultDest());
> +    Builder.SetInsertPoint(MaskBB);
> +    LookupBB = MaskBB;

I wonder if this mask checking code could be simpler. Look at
SwitchLookupTable::BuildLookup for the BitMapKind case.


You'll also need to update the table building code in
SwitchLookupTable::SwitchLookupTable. (I assume you did this locally,
otherwise it will assert when not having a default value to fill the
holes?)

 - Hans