[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

[Evan Cheng]

LGTM. Thanks.

Evan

On Sep 18, 2012, at 3:54 AM, Hans Wennborg <hans at chromium.org> wrote:

> On Thu, Sep 13, 2012 at 6:13 PM, Hans Wennborg <hans at chromium.org> wrote:
>> On Wed, Sep 12, 2012 at 8:26 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>>> I think this is a reasonable approach. Can you verify that it handles all the cases? That is, disabling the simplifycfg optimization generates the same code as simplifycfg + converting table back to switches?
>> 
>> I've polished the patch a bit and attached the new version. Yes, this
>> handles all the cases. The code is not going to be identical to if we
>> disable simplifycfg, e.g. if we had branch frequency metadata on the
>> original switch, that will be gone. It might also change which is
>> default case of the switch.
>> 
>> My question now is how to hook this up and test it. I guess we should
>> add a flag in TargetLoweringInfo to enable/disable it? I looked at
>> SupportJumpTables, but that's only used (i.e. set to false) by PPC for
>> JIT compilation. Which targets do we want to enable this for?
>> 
>> Once we know which targets to enable this for, I guess I should test
>> it by running llc for that target, and check that we don't emit the
>> lookup table?
>> 
>> Duncan: Doing the more general approach scares me a little bit. I
>> would prefer to use the more "narrow" approach of looking at the GEPs,
>> I believe it should cover all the lookup tables generated by my
>> original transform.
> 
> I've attached a new patch that turns the lookup tables back into
> switches for targets that don't support jump tables. That was the best
> hook in TargetLowering that I could find. It also has a test.
> 
> OK to commit?
> 
> Comments for hooking it up better and/or better testing are welcome.
> 
> Thanks,
> Hans
> 
> 
>>> On Sep 11, 2012, at 10:01 AM, Hans Wennborg <hans at chromium.org> wrote:
>>>> On Mon, Sep 10, 2012 at 7:19 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>>>>> The *right* longer term solution is to expose more target info to LLVM IR
>>>>> passes. It's going to be more and more important especially with recent
>>>>> interest in vectorization. I am fairly certain we'll start exposing some
>>>>> target "instruction cost" (to start) in 2013.
>>>>> 
>>>>> That said, I think we will need a short term compromise. Hans, can you
>>>>> experiment with moving this to codegenprep? I'd like to see what are the
>>>>> interaction with other IR optimizations.
>>>> 
>>>> I've been looking into moving this to codegenprep today. One of the
>>>> issues that came up is that if we move out of SimplifyCFG, we don't
>>>> get the benefit from some of the clean-ups it does, e.g. removing
>>>> unreachable blocks, merging blocks, etc. It seems that the codegen
>>>> passes manage to clean this up pretty well, but the output on the IR
>>>> level is not as good.
>>>> 
>>>> Another thing is that I couldn't find a way to run codegenprep via opt
>>>> so that TargetLowerInfo is available. I guess this means I wouldn't be
>>>> able to test my transformation via opt, but should use llc instead? Or
>>>> I guess I could turn the logic around and do my transformation
>>>> *unless* there is TLI available which says we shouldn't do it.
>>>> 
>>>> Talking to Duncan on IRC, he thinks we should not move this to
>>>> CodeGenPrepare, but instead look into converting the table look-ups
>>>> back into switches for those targets that need it, and that this would
>>>> benefit other loads from constants as well. We could do this quite
>>>> easily in CodeGenPrepare, I think. It would look something like the
>>>> attached (work in progress) patch.
>>>> 
>>>> Please let me know what you think.
>>>> 
>>>> Thanks,
>>>> Hans
>>>> <load_to_switch.patch>
> <load_to_switch3.patch>