>From their Discord server in reply to a question about whether such a
patch would be upstreamed: "I suspect this only works in gcc -O0
because of its AST-level "fold", which clang intentionally doesn't
implement. So probably not."
I hope that's enough information to resolve this patch. If any of you
need anything else, please let me know.
On Tue, Nov 21, 2023 at 12:28 PM Dan Hoffman <dhoff...@gmail.com> wrote:
>
> 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
> >
