On Thu, Jan 13, 2022 at 04:01:45PM +0100, Rasmus Villemoes wrote: > On 13/01/2022 13.52, Tom Rini wrote: > > On Thu, Jan 13, 2022 at 08:56:02AM +0100, Rasmus Villemoes wrote: > >> On 12/01/2022 22.56, Tom Rini wrote: > >>> I also think I've seen cases where doing: > >>> if (CONFIG_EVALUATES_TO_ZERO) { > >>> ... > >>> } > >>> > >>> takes more space in the binary than an #ifdef does. > >> > >> Please provide a specific example. If CONFIG_EVALUATES_TO_ZERO is any > >> integer-constant-expression evaluating at compile-time to 0, gcc throws > >> away the whole block very early during parsing. If it doesn't, that's a > >> compiler bug, so let's please not make decisions based on > >> not-even-anecdotal data. > > > > OK. I believe it was commit 7856cd5a6dd6 ("Convert CONFIG_SYS_PCI_64BIT > > to Kconfig") a few platforms changed size > > Can you remember which ones? I'd like to see if I can reproduce. > > That said, that commit made the Kconfig symbol 'default y if PPC'. Are > you really sure all ppc-boards that set CONFIG_PCI also used to set > SYS_PCI_64BIT? > > And another thing I notice is that a lot of the #define removals remove > > #define CONFIG_SYS_PCI_64BIT > > and not > > #define CONFIG_SYS_PCI_64BIT 1 > > Now that doesn't matter for the places that test the definedness of > CONFIG_SYS_PCI_64BIT, because kconfig either doesn't define it or define > it with value 1. But it does matter for (the single) IS_ENABLED() use, > because IS_ENABLED(bla) evaluates to 1 if and only if bla expands to 1. > Or rather, if and only if __ARG_PLACEHOLDER_ concatenated with the > expansion of bla in turn expands to "0, " - which only happens if we hit > the __ARG_PLACEHOLDER_1 macro. > > So when bla is defined with an empty expansion, for the purpose of > IS_ENABLED, it might as well not be defined or expand to 0 or to > gobbledygook. > > And when one knows what to look for, it's easy to demonstrate: > > $ export ARCH=ppc > $ export CROSS_COMPILE=powerpc-linux-gnu- > $ git checkout 7856cd5a6dd6~1 > $ make T1042D4RDB_defconfig > $ make drivers/pci/pci-uclass.i > $ grep -C3 'beyond the 32-bit boundary' drivers/pci/pci-uclass.i > > if (!(0) && > type == 0x00000000 && ((u32)(((pci_addr) >> 16) >> 16))) { > ({ if (0) printf(" - beyond the 32-bit boundary, ignoring\n"); }); > continue; > } > > $ git checkout 7856cd5a6dd6 > $ make T1042D4RDB_defconfig > $ make drivers/pci/pci-uclass.i > $ grep -C3 'beyond the 32-bit boundary' drivers/pci/pci-uclass.i > > if (!(1) && > type == 0x00000000 && ((u32)(((pci_addr) >> 16) >> 16))) { > ({ if (0) printf(" - beyond the 32-bit boundary, ignoring\n"); }); > continue; > } > > Whether that change makes the generated code smaller or larger I can't > say, but it's certainly not a nop semantically. [Of course, the change > is for the better, as the generated code now matches the intention; > previously 64 bit pci addresses would be ignored for the boards that had > an empty definition of CONFIG_SYS_PCI_64BIT.] > > But it has nothing whatsoever to do with whether gcc is capable of > throwing away a whole "if (0)" block. But I will believe that other > Kconfig conversions have been bit by the same issue, making it _seem_ > like IS_ENABLED() is somehow at fault and #ifdefs are "better".
Yes, that's what happened there, thanks for digging in and explaining. What I really meant by "better" was "same size when converting" which is important when migrating a ton of machines I can only very partially test. -- Tom
signature.asc
Description: PGP signature