From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid() Date: Tue, 12 Nov 2013 13:09:06 +0100 Message-ID: <52821A62.2050001@redhat.com> References: <1384204922-8250-1-git-send-email-peter.maydell@linaro.org> <5281580D.7060305@redhat.com> <52816422.8060002@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Anthony Liguori , QEMU Developers , Gleb Natapov , kvm-devel , Patch Tracking , Andreas Tobler , Anthony Liguori To: Peter Maydell Return-path: Received: from mail-ee0-f51.google.com ([74.125.83.51]:49151 "EHLO mail-ee0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752098Ab3KLMJN (ORCPT ); Tue, 12 Nov 2013 07:09:13 -0500 Received: by mail-ee0-f51.google.com with SMTP id t10so3056868eei.38 for ; Tue, 12 Nov 2013 04:09:12 -0800 (PST) In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: Il 12/11/2013 12:07, Peter Maydell ha scritto: > On 11 November 2013 23:21, Anthony Liguori wrote: >> We're not talking about something obscure here. It's eliminating an >> if(0) block. > > No, we're not talking about a simple "if (0)" expression. > What we had in this case was > if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) { > break; > } > [stuff using kvm_arch_get_supported_cpuid()] > > [where kvm_enabled() is #defined to constant-0]. > > For the compiler to eliminate this we are relying on: > * dead-code elimination of code following a 'break' > statement in a case block > * constant-folding of "something || 1" to 1 > * the compiler having done enough reasoning to be > sure that env is not NULL Yes, it's not trivial, but there are simpler ways to do it. For example there is no need to make sure that env is non-NULL, only to see that "something || 1" is never zero and thus "if (x) y;" is just "(void)x; y;". This seems easier to me than DCE after "break" which clang is able to do. Indeed, NULL checks (except perhaps very simple checks like &x != NULL) are not something I'd expect the compiler to optimize out at -O0. > You and Paolo have both mentioned "doing checks > at compile time" but why is this particular > detail of our code worth checking in that way > at the expense of reliably being able to compile? > > As it happens, having a stub function that returns > 0 would simplify several bits of code that currently > do: > > if (kvm_enabled() && cpu->enable_pmu) { > KVMState *s = cs->kvm_state; > > *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX); > *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX); > *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX); > *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX); > } else { > *eax = 0; > *ebx = 0; > *ecx = 0; > *edx = 0; > } > > because you could get rid of the else block. No, you couldn't because the "else" branch runs for "kvm_enabled() && !cpu->enable_pmu" too. Relying on kvm_arch_get_supported_cpuid to return 0 would only make code harder to review, and the above example shows this fact very well. kvm_arch_get_supported_cpuid abort()-ing would be fine, but it would also make code harder to review, because you cannot rely anymore on the compiler-linker combo to filter out the most obvious cases of forgetting about TCG's existence. > Or you could #ifdef CONFIG_KVM this code section, > as we already do for some of the more complicated > bits of code that call this function. That would > work too. Yes, but it wouldn't be _right_. Most of the code below the "break" is potentially applicable to TCG, even if it is not used now. Paolo