From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id 48A90396EC49 for ; Wed, 20 Jan 2021 15:58:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 48A90396EC49 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-146-VmEPLrhlMs-pLFq_zTI3Ww-1; Wed, 20 Jan 2021 10:58:39 -0500 X-MC-Unique: VmEPLrhlMs-pLFq_zTI3Ww-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 769C01005504; Wed, 20 Jan 2021 15:58:38 +0000 (UTC) Received: from oldenburg.str.redhat.com (ovpn-112-110.ams2.redhat.com [10.36.112.110]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 675EA6E405; Wed, 20 Jan 2021 15:58:37 +0000 (UTC) From: Florian Weimer To: "H.J. Lu via Libc-alpha" Subject: Re: V2 [PATCH] : Remove the C preprocessor magic References: <20201225162451.1657088-1-hjl.tools@gmail.com> <87h7nc6ikc.fsf@oldenburg.str.redhat.com> <20210120144339.GA1866418@gmail.com> Date: Wed, 20 Jan 2021 16:58:30 +0100 In-Reply-To: <20210120144339.GA1866418@gmail.com> (H. J. Lu via Libc-alpha's message of "Wed, 20 Jan 2021 06:43:39 -0800") Message-ID: <87turb4n2x.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jan 2021 15:58:43 -0000 * H. J. Lu via Libc-alpha: > On Wed, Jan 20, 2021 at 10:53:07AM +0100, Florian Weimer wrote: >> * H. J. Lu via Libc-alpha: >> >> > +enum >> > +{ >> > + COMMON_CPUID_INDEX_1 = 0, >> > + COMMON_CPUID_INDEX_7, >> > + COMMON_CPUID_INDEX_80000001, >> > + COMMON_CPUID_INDEX_D_ECX_1, >> > + COMMON_CPUID_INDEX_80000007, >> > + COMMON_CPUID_INDEX_80000008, >> > + COMMON_CPUID_INDEX_7_ECX_1, >> > + COMMON_CPUID_INDEX_19, >> > + /* Keep the following line at the end. */ >> > + COMMON_CPUID_INDEX_MAX >> > +}; >> >> COMMON_CPUID_INDEX_MAX should not be in the public header because it >> subject to changes. > > Fixed. > >> >> > +struct cpuid_registers >> > +{ >> > + unsigned int eax; >> > + unsigned int ebx; >> > + unsigned int ecx; >> > + unsigned int edx; >> > +}; >> >> Should the non-public interfaces use __? > > Moved to sysdeps/x86/include/cpu-features.h. > >> >> > +enum cpu_features_kind >> > +{ >> > + arch_kind_unknown = 0, >> > + arch_kind_intel, >> > + arch_kind_amd, >> > + arch_kind_zhaoxin, >> > + arch_kind_other >> > +}; >> > + >> > +struct cpu_features_basic >> > +{ >> > + enum cpu_features_kind kind; >> > + int max_cpuid; >> > + unsigned int family; >> > + unsigned int model; >> > + unsigned int stepping; >> > +}; >> >> Should we really expose all this? It's not documented. enum >> cpu_features_kind doesn't seem to be compatible with how people use >> virtualization. > > Moved to sysdeps/x86/include/cpu-features.h. > >> >> > diff --git a/sysdeps/x86/dl-get-cpu-features.c b/sysdeps/x86/dl-get-cpu-features.c >> > index 349472d99f..4636d9f4a7 100644 >> > --- a/sysdeps/x86/dl-get-cpu-features.c >> > +++ b/sysdeps/x86/dl-get-cpu-features.c >> > @@ -46,9 +46,9 @@ __ifunc (__x86_cpu_features, __x86_cpu_features, NULL, void, >> > #undef __x86_get_cpu_features >> > >> > const struct cpu_features * >> > -__x86_get_cpu_features (unsigned int max) >> > +__x86_get_cpu_features (unsigned int index) >> > { >> > - if (max > COMMON_CPUID_INDEX_MAX) >> > + if (index > COMMON_CPUID_INDEX_MAX * 8 * sizeof (unsigned int) * 4) >> > return NULL; >> > return &GLRO(dl_x86_cpu_features); >> > } >> >> This should work in principle (but I haven't verified the boundary >> condition). >> >> Could you pass the COMMON_CPUID_INDEX_* value (without the register and >> bit selectors)? Then more calls can be subject to common subexpression >> elimination in the caller, leading to more compact code. > > My current public API uses a single enum index for each CPU feature > detection. Use 2 indices for each CPU feature makes it less user > friendly. Internally, we use the C preprocessor magic so that more > calls can be subject to common subexpression elimination in the caller, > leading to more compact code for glibc. I had this in mind for the implementation: struct cpuid_feature { unsigned int cpuid_array[4]; unsigned int usable_array[4]; }; const struct cpuid_feature * __x86_get_cpu_features (unsigned int leaf) { const struct cpu_feature future = { }; if (leaf < CPUID_INDEX_MAX) return &GLRO(dl_x86_cpu_features).features[leaf]; else return &future; } And in the header: unsigned int leaf = index / (8 * sizeof (unsigned int) * 4); const struct cpuid_feature * __x86_get_cpu_features (unsigned int leaf) __attribute__ ((pure)); static inline const struct cpuid_feature * __x86_get_cpu_leaf (unsigned int index) { return __x86_get_cpu_features (index / (8 * sizeof (unsigned int) * 4)); } #define HAS_CPU_FEATURE(name) \ (x86_cpu_has_feature (__x86_get_cpu_leaf (x86_cpu_##name), \ x86_cpu_##name)) #define HAS_CPU_FEATURE(name) \ (x86_cpu_is_usable (__x86_get_cpu_leaf (x86_cpu_##name), \ x86_cpu_##name)) I expect that the index arguments to __x86_get_cpu_features are constant-folded by GCC, and many of the __x86_get_cpu_features calls can be CSE'ed as a result. In the public interface, the max_cpuid field appears redundant, so this is why I think this will work. Thanks, Florian -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill