On Tue, Oct 1, 2019 at 2:28 AM Will Deacon [off-list ref] wrote:
Hi Nick,
On Mon, Sep 30, 2019 at 02:50:10PM -0700, Nick Desaulniers wrote:
quoted
So __attribute__((always_inline)) doesn't guarantee that code will be
inlined. For instance in LLVM's inliner, it asks/answers "should I
inline" and "can I inline." "Should" has to do with a cost model, and
is very heuristic-y. "Can" has more to do with the transforms, and
whether they're all implemented and safe. If you if you say
__attribute__((always_inline)), the answer to "can I inline this" can
still be *no*. The only way to guarantee inlining is via the C
preprocessor. The only way to prevent inlining is via
__attribute__((no_inline)). inline and __attribute__((always_inline))
are a heuristic laden mess and should not be relied upon. I would
also look closely at code that *requires* inlining or the lack there
of to be correct. That the kernel no longer compiles at -O0 is not a
good thing IMO, and hurts developers that want a short
compile/execute/debug cycle.
In this case, if there's a known codegen bug in a particular compiler
or certain versions of it, I recommend the use of either the C
preprocessor or __attribute__((no_inline)) to get the desired behavior
localized to the function in question, and for us to proceed with
Masahiro's cleanup.
Hmm, I don't see how that would help. The problem occurs when things
are moved out of line by the compiler (see below).
It's being moved out of line because __attribute__((always_inline)) or
just inline provide no guarantees that outlining does not occur. It
would help to make functions that need to be inlined macros, because
the C preprocessor doesn't have that issue.
quoted
The comment above the use of CONFIG_OPTIMIZE_INLINING in
include/linux/compiler_types.h says:
* Force always-inline if the user requests it so via the .config.
Which makes me grimace (__attribute__((always_inline)) doesn't *force*
anything as per above), and the idea that forcing things marked inline
to also be __attribute__((always_inline)) is an "optimization" (re:
the name of the config; CONFIG_OPTIMIZE_INLINING) is also highly
suspect. Aggressive inlining leads to image size bloat, instruction
cache and register pressure; it is not exclusively an optimization.
Agreed on all of this, but the fact remains that GCC has been shown to
*miscompile* the arm64 kernel with CONFIG_OPTIMIZE_INLINING=y. Please,
look at this thread:
https://www.spinics.net/lists/arm-kernel/msg730329.html
https://www.spinics.net/lists/arm-kernel/msg730512.html
GCC decides to pull an atomic operation out-of-line and, in doing so,
If the function is incorrect unless inlined, use a macro.
gets the register allocations subtly wrong when passing a 'register'
variable into an inline asm. I would like to avoid this sort of thing
happening, since it can result in really nasty bugs that manifest at
runtime and are extremely difficult to debug, which is why I would much
prefer not to have this option on by default for arm64. I sent a patch
already:
https://lkml.kernel.org/r/20190930114540.27498-1-will@kernel.org (local)
and I'm happy to spin a v2 which depends on !CC_IS_CLANG as well.
For small things like whether we mark a function always_inline or not,
I think it's simpler to just keep the code consistent between
compilers, even if it's to work around a bug in one compiler. A
comment in the code would be sufficient.
Reducing the instruction cache footprint is great, but not if the
resulting code is broken!
You don't have to convince compiler folks about correctness. ;)
Correctness trumps all, especially performance.
--
Thanks,
~Nick Desaulniers