Re: [RFC PATCH] pre-process: add __VA_OPT__ support
Al Viro <[email protected]> Thu, 19 Mar 2026 03:53:24 +0000
| Newsgroups | org.kernel.vger.linux-sparse |
|---|---|
| Message-ID | <20260319035324.GG3836593@ZenIV> |
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 ;-/