From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1340 invoked by alias); 2 May 2012 17:44:26 -0000 Received: (qmail 1331 invoked by uid 22791); 2 May 2012 17:44:25 -0000 X-SWARE-Spam-Status: No, hits=-5.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-qa0-f47.google.com (HELO mail-qa0-f47.google.com) (209.85.216.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 02 May 2012 17:44:04 +0000 Received: by qabg1 with SMTP id g1so2984943qab.20 for ; Wed, 02 May 2012 10:44:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding:x-system-of-record :x-gm-message-state; bh=kwxdcwYq3DeuQ4pDxF13/lM0rePIlPgN3+NfmT+NlwQ=; b=ZZO5dqRybiozJcbGmaZ5xZPfDmbjP6u9YTGEQD2en2OYWlQUjIwgi+e2DtmF503VFf EPxSsmu8nMLOnmarzRS/RMkgxl6v7sBMBCijn/blGnrApoH0Zy6vDw584dUPbwGbCEAE pnggWC4amOhqhhM4eB2dp2N/bY27mrY28uqTptnP4JmhcYbkRIuF2LnMcj0Sx2lK3sbw yzMirSsM3gHufRE9T+k/gPTZMnwN3BdgQ4cpz3HwE2p1fppYo4N/awxzkfqq/BjPzzK4 XOGuoRUMfOtQs3oTUvr2cW918jXuoRp8FgR2zZPSxm2Z+YW79xrOmsr+7J4OdDYXNKOE q5Gw== Received: by 10.60.20.38 with SMTP id k6mr1295430oee.26.1335980643784; Wed, 02 May 2012 10:44:03 -0700 (PDT) MIME-Version: 1.0 Received: by 10.60.20.38 with SMTP id k6mr1295393oee.26.1335980643556; Wed, 02 May 2012 10:44:03 -0700 (PDT) Received: by 10.182.147.104 with HTTP; Wed, 2 May 2012 10:44:03 -0700 (PDT) In-Reply-To: References: <20120307004630.A503DB21B6@azwildcat.mtv.corp.google.com> Date: Wed, 02 May 2012 17:44:00 -0000 Message-ID: Subject: Re: User directed Function Multiversioning via Function Overloading (issue5752064) From: Sriraman Tallam To: "H.J. Lu" Cc: Richard Guenther , Jan Hubicka , Uros Bizjak , reply@codereview.appspotmail.com, gcc-patches@gcc.gnu.org, David Li Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true X-Gm-Message-State: ALoCoQl71H31YotQCZqr4i+rJK6Bo5+PIW2xAOavKNT1RCA46QD6XgbGaSqd5ATVNdiVBXQHjPiDQQjfiw8t8UFuIllooy09fP8Q/boBDC4IS/eB9DjG6SA9wL4LzrKpi+CezVbFx4i7fVFUyMXJlLLTmVNTwbfPQA== X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2012-05/txt/msg00153.txt.bz2 On Wed, May 2, 2012 at 9:05 AM, H.J. Lu wrote: > On Wed, May 2, 2012 at 8:08 AM, Sriraman Tallam wro= te: >> On Wed, May 2, 2012 at 6:42 AM, H.J. Lu wrote: >>> On Tue, May 1, 2012 at 7:45 PM, Sriraman Tallam w= rote: >>>> Hi H.J, >>>> >>>> =A0 Done now. Patch attached. >>>> >>>> Thanks, >>>> -Sri. >>>> >>>> On Tue, May 1, 2012 at 5:08 PM, H.J. Lu wrote: >>>>> On Tue, May 1, 2012 at 4:51 PM, Sriraman Tallam = wrote: >>>>>> Hi, >>>>>> >>>>>> New patch attached, updated test case and fixed bugs related to >>>>>> __PRETTY_FUNCTION_. >>>>>> >>>>>> Patch also available for review here: =A0http://codereview.appspot.c= om/5752064 >>>>> >>>>> @@ -0,0 +1,39 @@ >>>>> +/* Simple test case to check if Multiversioning works. =A0*/ >>>>> +/* { dg-do run } */ >>>>> +/* { dg-options "-O2 -fPIC" } */ >>>>> + >>>>> +#include >>>>> + >>>>> +int foo (); >>>>> +int foo () __attribute__ ((target("arch=3Dcorei7,sse4.2,popcnt"))); >>>>> +/* The target operands in this declaration and the definition are re= -ordered. >>>>> + =A0 This should still work. =A0*/ >>>>> +int foo () __attribute__ ((target("ssse3,avx2"))); >>>>> + >>>>> +int (*p)() =3D &foo; >>>>> +int main () >>>>> +{ >>>>> + =A0return foo () + (*p)(); >>>>> +} >>>>> + >>>>> +int foo () >>>>> +{ >>>>> + =A0return 0; >>>>> +} >>>>> + >>>>> +int __attribute__ ((target("arch=3Dcorei7,sse4.2,popcnt"))) >>>>> +foo () >>>>> +{ >>>>> + =A0assert (__builtin_cpu_is ("corei7") >>>>> + =A0 =A0 =A0 =A0 && __builtin_cpu_supports ("sse4.2") >>>>> + =A0 =A0 =A0 =A0 && __builtin_cpu_supports ("popcnt")); >>>>> + =A0return 0; >>>>> +} >>>>> + >>>>> +int __attribute__ ((target("avx2,ssse3"))) >>>>> +foo () >>>>> +{ >>>>> + =A0assert (__builtin_cpu_supports ("avx2") >>>>> + =A0 =A0 =A0 =A0 && __builtin_cpu_supports ("ssse3")); >>>>> + =A0return 0; >>>>> +} >>>>> >>>>> This test will pass if >>>>> >>>>> int foo () >>>>> { >>>>> =A0return 0; >>>>> } >>>>> >>>>> is selected on processors with AVX. =A0The run-time test should >>>>> check that the right function is selected on the target processor, >>>>> not the selected function matches the target attribute. You can >>>>> do it by returning different values for each foo and call cpuid >>>>> to check if the right foo is selected. >>>>> >>>>> You should add a testcase for __builtin_cpu_supports to check >>>>> all valid arguments. >>>>> >>>>> -- >>>>> H.J. >>> >>> 2 questions: >>> >>> 1. =A0Since AVX > SSE4 > SSSE3 > SSE3 > SSE2 > SSE, with >>> foo for AVX and SSE3, on AVX processors, which foo will be >>> selected? >> >> foo for AVX will get called since that appears ahead. >> >> The dispatching is done in the same order in which the functions are >> specified. If, potentially, two foo versions can be dispatched for an >> architecture, the first foo will get called. =A0There is no way right >> now to specify the order in which the dispatching should be done. > > This is very fragile. =A0We know ISAs and processors. =A0The source > order should be irrelevant. I am not sure it is always possible keep this dispatching unambiguous to the user. It might be better to let the user specify a priority for each version to control the order of dispatching. Still, one way to implement what you said is to assign a significance number to each ISA, where the number of sse4 > sse, for instance. Then, the dispatching can be done in the descending order of significance. What do you think? I thought about this earlier and I was thinking along the lines of letting the user specify a priority for each version, when there is ambiguity. > >> >>> 2. =A0I don't see any tests for __builtin_cpu_supports ("XXX") >>> nor __builtin_cpu_is ("XXX"). =A0I think you need tests for >>> them. >> >> This is already there as part of the previous CPU detection patch that >> was submitted. Please see gcc.target/i386/builtin_target.c. Did you >> want something else? > > gcc.target/i386/builtin_target.c doesn't test if __builtin_cpu_supports (= "XXX") > and __builtin_cpu_is ("XXX") are implemented correctly. Oh, you mean like doing a CPUID again in the test case itself and checking,= ok. Thanks, -Sri. > > > -- > H.J.