I'm writing a patch to clang's constant folding to address this case
(doesn't seem too difficult). I'll either follow up with a link to
some submissions I've made or a bug report on the project describing
the issue.
On Tue, Nov 21, 2023 at 10:15 AM Eric Blake <ebl...@redhat.com> wrote:
>
> On Mon, Nov 20, 2023 at 11:20:52AM +0100, Philippe Mathieu-Daudé wrote:
> > (Cc'ing Eric)
> >
> > On 20/11/23 10:28, Michael S. Tsirkin wrote:
> > > On Sun, Nov 19, 2023 at 07:34:58PM -0600, Dan Hoffman wrote:
> > > > As far as I can tell, yes. Any optimization level above O0 does not
> > > > have this
> > > > issue (on this version of Clang, at least)
> > >
> > > Aha, this is with -O0. That makes sense.
> >
> > But then, why the other cases aren't problematic?
> >
> > $ git grep -E ' (&&|\|\|) !?kvm_enabled'
> > hw/arm/boot.c:1228: assert(!(info->secure_board_setup && kvm_enabled()));
>
> This one's obvious; no kvm_*() calls in the assert.
>
> > hw/i386/microvm.c:270: (mms->rtc == ON_OFF_AUTO_AUTO &&
> > !kvm_enabled())) {
>
> Ones like this require reading context to see whether the if() block
> guarded any kvm_*() calls for the linker to elide - but still a fairly
> easy audit.
>
> > > >
> > > > I'm surprised the order of conditions matters for code elision...
> > > >
> > > > > Signed-off-by: Daniel Hoffman <dhoff...@gmail.com>
> > > > > ---
> > > > > hw/i386/x86.c | 15 ++++++++++++---
> > > > > 1 file changed, 12 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > > > > index b3d054889bb..2b6291ad8d5 100644
> > > > > --- a/hw/i386/x86.c
> > > > > +++ b/hw/i386/x86.c
> > > > > @@ -131,8 +131,12 @@ void x86_cpus_init(X86MachineState *x86ms,
> > > > int
> > > > default_cpu_version)
> > > > > /*
> > > > > * Can we support APIC ID 255 or higher? With KVM, that
> > > > requires
> > > > > * both in-kernel lapic and X2APIC userspace API.
> > > > > + *
> > > > > + * kvm_enabled() must go first to ensure that kvm_*
> > > > references are
> > > > > + * not emitted for the linker to consume (kvm_enabled() is
> > > > > + * a literal `0` in configurations where kvm_* aren't
> > > > defined)
> > > > > */
> > > > > - if (x86ms->apic_id_limit > 255 && kvm_enabled() &&
> > > > > + if (kvm_enabled() && x86ms->apic_id_limit > 255 &&
> > > > > (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
>
> Indeed, if clang -O0 treats 'if (cond1 && 0 && cond2)' differently
> than 'if (0 && cond1 && cond2)' for purposes of eliding the code for
> cond2, that seems like a blatant missed optimization that we should be
> reporting to the clang folks.
>
> While this patch may solve the immediate issue, it does not scale - if
> we ever have two separate guards that can both be independently
> hard-coded to 0 based on configure-time decisions, but which are both
> used as guards in the same expression, it will become impossible to
> avoid a '(cond1 && 0 && cond2)' scenario across all 4 possible
> configurations of those two guards.
>
> I have no objection to the patch, but it would be nice if the commit
> message could point to a clang bug report, if one has been filed.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization: qemu.org | libguestfs.org
>
