[llvm-commits] [Patch] Use TargetTransformInfo to control switch-to-lookup table transform

[Hans Wennborg]

On Mon, Oct 29, 2012 at 4:48 PM, Duncan Sands <baldrick at free.fr> wrote:
> Hi Hans,
>
>
> On 29/10/12 17:31, Hans Wennborg wrote:
>>
>> On Mon, Oct 29, 2012 at 3:29 PM, Duncan Sands <baldrick at free.fr> wrote:
>>>>
>>>> When the switch-to-lookup tables transform landed in SimplifyCFG, it
>>>> was pointed out [1] that this could be inappropriate for some targets.
>>>> Since there was no way at the time for the pass to know anything about
>>>> the target, an awkward reverse-transform was added in CodeGenPrepare
>>>> that turned lookup tables back into switches for some targets.
>>>>
>>>> The attached patch uses the new TargetTransformInfo to determine if a
>>>> switch should be transformed, and removes
>>>> CodeGenPrepare::ConvertLoadToSwitch. Please take a look!
>>>
>>>
>>>
>>> personally I think it is completely wrong to have codegen concepts such
>>> as
>>> ISD::BR_JT and ISD::BRIND leaking into the IR level:
>>>
>>>> +bool ScalarTargetTransformImpl::shouldBuildLookupTables() const {
>>>> +  return TLI->supportJumpTables() &&
>>>> +      (TLI->isOperationLegalOrCustom(ISD::BR_JT, MVT::Other) ||
>>>> +       TLI->isOperationLegalOrCustom(ISD::BRIND, MVT::Other));
>>
>>
>> But as Hal pointed out below, those codegen concepts don't leak into
>> the IR level. The only thing that goes into the IR level is a "yes" or
>> "no" on whether the IR level should do this switch transformation or
>> not. Is that what you're opposing, or are you opposing the way
>> ScalarTargetTransformImpl::shouldBuildLookupTables is implemented?
>
>
> if I'm not mistaken, you added the above routine using ISD opcodes to
> Local.h, which is an IR level file.  In my opinion all such uses of ISD
> opcodes and
> such like should be restricted to the files implementing target information
> (which should export shouldBuildLookupTables as part of its interface).

Hmm, I'm confused. In Local.h, I'm only updating the signature of
SimplifyCFG to include a TargetTransformInfo parameter.

The shouldBuildLookupTables function, which uses the ISD opcodes, is
in lib/Target/TargetTransformImpl.cpp.

Thanks,
Hans