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

[Duncan Sands]

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).

Ciao, Duncan.