From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Eric Zhang <zxh@xh-zhang.com>,
linux-sparse@vger.kernel.org, dan.carpenter@linaro.org,
chriscli@google.com, ben.dooks@codethink.co.uk,
rf@opensource.cirrus.com
Subject: Re: [RFC PATCH] pre-process: add __VA_OPT__ support
Date: Thu, 19 Mar 2026 03:53:24 +0000 [thread overview]
Message-ID: <20260319035324.GG3836593@ZenIV> (raw)
In-Reply-To: <CAHk-=wghBU1bB6E889nv5fg+40b4_n5WnA0h3bVh32EKw0zE2w@mail.gmail.com>
On Mon, Mar 16, 2026 at 09:42:01AM -0700, Linus Torvalds wrote:
> I have tested that branch on a few trivial cases, and it looks good to me.
>
> I did write a long rant about how I hate cpp tricks and wish we had a
> few simple extensions (__VA_COUNT__ would be the most simple one,
> because COUNT_ARGS() is disgusting), but it is what it is, and this
> makes things better. So I decided to just delete my rant.
Speaking of rants: gcc code generation in general and around bitfields.
Example: in collect_arg() we have
next->pos.stream = pos->stream;
next->pos.line = pos->line;
next->pos.pos = pos->pos;
next->pos.newline = 0;
in a loop, pos is declared const struct position *. Generates the
following horror:
movzwl 2(%r12), %edx
movl (%r12), %ecx
leaq 8(%rax), %rbp
shrw $6, %dx
andl $1048512, %ecx
movzwl %dx, %edx
salq $22, %rdx
orq %rcx, %rdx
movl 4(%r12), %ecx
andl $2147483647, %ecx
salq $32, %rcx
orq %rcx, %rdx
movq (%rax), %rcx
andq %r14, %rcx
orq %rcx, %rdx
movq %rdx, (%rax)
r12 is 'pos', rax - 'next', r14 comes from
movabsq $-9223372036852678593, %r14
in the beginning of the function (0x800000000020003f). Note that
*everything* prior to the last 4 insns is equivalent to
rbp = &(struct token *)rax->next
rdx = *(u64 *)r12 & 0x7fffffffffc0xfffc0
written in a really convoluted way.
OK, so it doesn't figure out it could bloody well calculate that rdx
value once and store in some register (the same r12, for that matter).
Let's make it simple for the damn thing - pass struct position instead
of struct position *; what we get is
movq %r12, %rdx
leaq 8(%rax), %rbp
movabsq $9223372032559808512, %rcx
andl $1048512, %edx
andq %r12, %rcx
orq %r14, %rdx
orq %rcx, %rdx
movabsq $-9223372036852678593, %rcx
andq (%rax), %rcx
orq %rcx, %rdx
movq %rdx, (%rax)
What the hell? No, really - it's
rdx = r12;
rbp = &(struct token *)rax->next;
rcx = 0x7fffffff00000000;
rdx &= 0xfffc0;
rcx &= r12;
rdx |= rcx;
followed by
rcx = 0x800000000020003f & *(u64 *)rax;
rdx |= rcx;
*(u64 *)rax = rdx;
Leaving aside the utility of repeating the same calculation on each
iteration of the loop, figuring out that
(0x7fffffff00000000 & r12) | (0xfffc0 & r12)
is equal to
0x7fffffffffc0fffc0 & r12
ought to be within the abilities of the damn compiler - and it *is*
loading a 64bit constant into rcx as it is. What's more, it's not
a preference to using 64bit constants with lower 32 bits clear -
another movabsq in the same chunk is not of that form.
Perhaps it's an explicit store of 0 to ->newline that does it?
Clearing pos.newline in the beginning and have
next->pos.newline = pos.newline;
instead of zeroing in the loop does not change anything (other than
worse register allocation). Moving zeroing of pos.newline into the
caller finally gets that calculation out of loop... and messes with
the register allocation in the caller, which is inlined into expand(),
along with substitute(), do_argument() and quite a few other things.
Granted, collect_arg() is not particularly hot, but... ouch.
I really, really don't like the handling of bitfields ;-/
next prev parent reply other threads:[~2026-03-19 3:50 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1771930766.git.dan.carpenter@linaro.org>
2026-02-24 11:07 ` [PATCH] sparse: add support for __VA_OPT__ Dan Carpenter
2026-02-24 11:16 ` Ben Dooks
2026-02-24 11:56 ` Dan Carpenter
2026-02-24 12:42 ` Richard Fitzgerald
2026-02-24 13:15 ` Ben Dooks
2026-02-25 2:39 ` Chris Li
2026-02-25 3:36 ` Al Viro
2026-02-25 5:29 ` [RFC PATCH] pre-process: add __VA_OPT__ support Eric Zhang
2026-02-25 6:40 ` Al Viro
2026-02-25 7:27 ` Al Viro
2026-02-25 8:14 ` Eric Zhang
2026-02-25 22:18 ` Al Viro
2026-02-26 7:29 ` Al Viro
2026-03-16 6:56 ` Al Viro
2026-03-16 7:03 ` [PATCH 01/21] split copy() into "need to copy" and "can move in place" cases Al Viro
2026-03-16 7:03 ` [PATCH 02/21] expand and simplify the call of dup_token() in copy() Al Viro
2026-03-16 7:03 ` [PATCH 03/21] more dup_token() optimizations Al Viro
2026-03-16 7:03 ` [PATCH 04/21] parsing #define: saner handling of argument count, part 1 Al Viro
2026-03-16 7:03 ` [PATCH 05/21] simplify collect_arguments() and fix error handling there Al Viro
2026-03-16 7:04 ` [PATCH 06/21] try_arg(): don't use arglist for argument name lookups Al Viro
2026-03-16 7:04 ` [PATCH 07/21] make expand_has_...() responsible for expanding its argument Al Viro
2026-03-16 7:04 ` [PATCH 08/21] preparing to change argument number encoding for TOKEN_..._ARGUMENT Al Viro
2026-03-16 7:04 ` [PATCH 09/21] steal 2 bits from argnum for argument kind Al Viro
2026-03-16 7:04 ` [PATCH 10/21] on-demand argument expansion Al Viro
2026-03-16 7:04 ` [PATCH 11/21] kill create_arglist() Al Viro
2026-03-16 7:04 ` [PATCH 12/21] stop mangling arglist, get rid of TOKEN_ARG_COUNT Al Viro
2026-03-16 7:04 ` [PATCH 13/21] deal with ## on arguments separately Al Viro
2026-03-16 7:04 ` [PATCH 14/21] preparations for __VA_OPT__ support: reshuffle argument slot assignments Al Viro
2026-03-16 7:04 ` [PATCH 15/21] pre-process.c: split try_arg() Al Viro
2026-03-16 7:04 ` [PATCH 16/21] __VA_OPT__: parsing Al Viro
2026-03-16 7:04 ` [PATCH 17/21] expansion-time va_opt handling Al Viro
2026-03-16 7:04 ` [PATCH 18/21] merge(): saner handling of ->noexpand Al Viro
2026-03-16 7:04 ` [PATCH 19/21] simplify the calling conventions of collect_arguments() Al Viro
2026-03-16 7:04 ` [PATCH 20/21] make expand_one_symbol() inline Al Viro
2026-03-16 7:04 ` [PATCH 21/21] substitute(): convert switch() into cascade of ifs Al Viro
2026-03-16 16:42 ` [RFC PATCH] pre-process: add __VA_OPT__ support Linus Torvalds
2026-03-19 3:53 ` Al Viro [this message]
2026-03-19 4:07 ` Linus Torvalds
2026-03-19 5:34 ` Al Viro
2026-03-17 7:41 ` Chris Li
2026-03-18 6:35 ` Eric Zhang
2026-02-25 7:05 ` [PATCH] sparse: add support for __VA_OPT__ Chris Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260319035324.GG3836593@ZenIV \
--to=viro@zeniv.linux.org.uk \
--cc=ben.dooks@codethink.co.uk \
--cc=chriscli@google.com \
--cc=dan.carpenter@linaro.org \
--cc=linux-sparse@vger.kernel.org \
--cc=rf@opensource.cirrus.com \
--cc=torvalds@linux-foundation.org \
--cc=zxh@xh-zhang.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox