[llvm-commits] [llvm] r163302 - in /llvm/trunk: lib/Transforms/Utils/SimplifyCFG.cpp test/Transforms/SimplifyCFG/switch_create.ll test/Transforms/SimplifyCFG/switch_to_lookup_table.ll

[Hans Wennborg]

On Sat, Sep 8, 2012 at 11:04 PM, Evan Cheng <evan.cheng at apple.com> wrote:
> Sorry I'm late to this. I see the patch has been reviewed but I have some concerns.
>
> 1. There appear to be some stylistic issues still. e.g.
> +static GlobalVariable *BuildLookupTable(
> +    Module &M,
> +    uint64_t TableSize,
> +    ConstantInt *Offset,
> +    const std::vector<std::pair<ConstantInt*,Constant*> >& Results,
> +    Constant *DefaultResult,
> +    Constant **SingleResult) {
>
> LLVM convention is more like:
> +static GlobalVariable *BuildLookupTable(Module &M,
> +                                                                          uint64_t TableSize,
> ...
> +                                                                          Constant **SingleResult) {
>
> +  // Get the resulting values for the default case.
> +  {
> +    SmallVector<std::pair<PHINode*, Constant*>, 4> DefaultResultsList;
> +    if (!GetCaseResults(SI, SI->getDefaultDest(), &CommonDest, DefaultResultsList))
> +      return false;
> +    for (size_t I = 0, E = DefaultResultsList.size(); I != E; ++I) {
> +      PHINode *PHI = DefaultResultsList[I].first;
> +      Constant *Result = DefaultResultsList[I].second;
> +      DefaultResults[PHI] = Result;
> +      ResultTypes[PHI] = Result->getType();
> +    }
> +  }
>
> Why the extra { }?
>
> These are minor nitpicks, of course.
>
> 2. It's using std::vector, etc. rather than LLVM's ADT.

Thank you for the comments! I have addressed the above in r163491. I
kept the std::vector in BuildLookupTable(). I think these tables will
typically be larger than something to put in SmallVector, and also
that code is not on the hot path so I'm not worried about doing a heap
allocation there.

> 3. My biggest concern is all these FIXME's

I hope to address them one by one, but it was pointed out in the code
review that it seemed like a bad idea to try and do everything in one
patch.

> Have you look into integrating this optimization into SelectionDAGBuilder instead? It seems like it very well could benefit from having target information. I am not certain it's a good idea to have switch optimizations spread out among different passes.
>
> I'm not opposed to a different approach. For example, move all switch related optimizations into codegenprep. I've always been a bit annoyed by the way switch is lowered in selectiondag.

I guess the benefit from target information is allowing some targets
to disable this? You pointed out in the other thread that this might
be unfeasible for some GPU targets, and Owen replied in this thread
that it might be bad for some deeply embedded targets.

I'm not familiar with GPUs or deeply embedded systems. What are the
problems that my transformation is creating for them?

If having target information for this is absolutely necessary, I guess
moving it to CodeGenPrepare would be a good solution. But as Duncan
pointed out in the other thread, doing this optimization early enables
other IR optimizations to work on the simplified code. In particular,
doing this before inlining seems like a good idea because the inliner
will see the cost of the simplified code.

Thanks,
Hans