[google] Patch to support calling multi-versioned functions via new GCC builtin. (issue4440078)

[Richard Guenther]

On Mon, May 2, 2011 at 6:41 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Mon, May 2, 2011 at 2:11 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Fri, Apr 29, 2011 at 6:23 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> Here is the background for this feature:
>>>
>>> 1) People relies on function multi-version to explore hw features and
>>> squeeze performance, but there is no standard ways of doing so, either
>>> a) using indirect function calls with function pointers set at program
>>> initialization; b) using manual dispatch at each callsite; b) using
>>> features like IFUNC.  The dispatch mechanism needs to be promoted to
>>> the language level and becomes the first class citizen;
>>
>> You are not doing that, you are inventing a new (crude) GCC extension.
>
> To capture the high level semantics and prevent user from lowering the
> dispatch calls into forms compiler can not recognize, language
> extension is the way to go.

I don't think so.  With your patch only two passes understand the new
high-level form, the rest of the gimple passes are just confused.

>>
>
>>> See above. Multi-way dispatch can be added similarly.
>>
>> Not with the specified syntax.  So you'd end up with _two_
>> language extensions.  That's bad (and unacceptable, IMHO).
>
> This part can be improved.
>
>>
>>>
>>>>
>>>> That's a nice idea, but why not simply process all functions with
>>>> a const attribute and no arguments this way?  IMHO
>>>>
>>>> int testsse (void) __attribute__((const));
>>>>
>>>> is as good and avoids the new attribute completely.  The pass would
>>>> also benefit other uses of this idiom (giving a way to have global
>>>> dynamic initializers in C).  The functions may not be strictly 'const'
>>>> in a way the compiler autodetects this attribute but it presents
>>>> exactly the promises to the compiler that allow this optimization.
>>>>
>>>> Thus, please split this piece out into a separate patch and use
>>>> the const attribute.
>>>
>>>
>>> Sounds good.
>>>
>
>>>
>>> 1) the function selection may happen in a different function;
>>
>> Well, sure.  I propose to have the above as lowered form.  If people
>> deliberately obfuscate code it's their fault.  Your scheme simply
>> makes that obfuscation impossible (or less likely possible).  So
>> 1) is not a valid argument.
>
> Lowering too early may defeat the purpose because
>
> 1) the desired optimization may not happen subject to many compiler
> heuristic changes;
> 2) it has other side effects such as wrong estimation of function size
> which may impact inlining

May, may ... so you say all this can't happen under any circumstance
with your special code and passes?  Which nobody will see benefit
from unless they rewrite their code?  Well, I say if we can improve
_some_ of the existing usages that's better than never doing wrong
on a new language extension.  One that I'm not convinced is the way
to go (you didn't address at all the inability to use float arguments
and the ABI issues with using variadic arguments - after all you
did a poor-mans language extension by using GCC builtins instead
of inventing a true one).

> 3) it limits the lowering into one form which may not be ideal  --
> with builtin_dispatch, after hoisting optimization, the lowering can
> use more efficient IFUNC scheme, for instance.

I see no reason why we cannot transform a switch-indirect-call
pattern into an IFUNC call.

>> My point is that such optimization is completely independent of
>> that dispatch thing.  The above could as well be a selection to
>> use different input arrays, one causing alias analysis issues
>> and one not.
>>
>> Thus, a __builtin_dispatch centric optimization pass is the wrong
>> way to go.
>
> I agree that many things can implemented in different ways, but a high
> level standard builtin_dispatch mechanism doing hoisting
> interprocedcurally is cleaner and simpler and targeted for function
> multi-versioning -- it does not depend on/rely on later phase's
> heuristic tunings to make the right things to happen. Function MV
> deserves this level of treatment as it will become more and more
> important for some users (e.g., Google).

But inventing a new language extension to benefit from whatever
improvements we implement isn't the obviously best choice.

>> Now, with FDO I'd expect the foo is inlined into bar (because foo
>> is deemed hot),
>
> That is a myth -- the truth is that there are other heuristics which
> can prevent this from happening.

Huh, sure.  That doesn't make my expectation a myth.

>> then you only need to deal with loop unswitching,
>> which should be easy to drive from FDO.
>
> Same here -- the loop body may not be well formed/recognized. The loop
> nests may not be perfectly nested, or other unswitching heuristics may
> block it from happening.  This is the common problem form many other
> things that get lowered too early. It is cleaner to make the high
> level transformation first in IPA, and let unswitching dealing with
> intra-procedural optimization.

If it's not well-formed inlining the call does not make it well-formed and
thus it won't be optimized well anyway.  Btw, for the usual cases I
have seen inlining isn't a transform that is worthwhile - transforming
to IFUNC would have been.

I'm not convinced at all.

Richard.