[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]

Thanks! Committed in r164206.

 - Hans

On Tue, Sep 18, 2012 at 9:16 PM, Evan Cheng <evan.cheng at apple.com> wrote:
> 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>
>