[Bug target/109903] New: Register misallocation in hand-crafted asm insn, no diagnostics produced

[Bug target/109903] New: Register misallocation in hand-crafted asm insn, no diagnostics produced

[dimitri.gorokhovik at free dot fr]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109903

            Bug ID: 109903
           Summary: Register misallocation in hand-crafted asm insn, no
                    diagnostics produced
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dimitri.gorokhovik at free dot fr
  Target Milestone: ---

For the source code:

#if NO_BUG
unsigned long f (unsigned long a)
{
  unsigned long v;

  asm ("mrs %[_1], pmccntr_eli0\n"
       "\tldr x1, [%[_2]]\n"
       : [_1] "=r" (v)
       : [_2] "r" (a)
       );

  return v;
}


#else // BUG

extern void g(unsigned long);
void f (unsigned long a)
{
  unsigned long v;

  asm ("mrs %[_1], pmccntr_eli0\n"
       "\tldr x1, [%[_2]]\n"
       : [_1] "=r" (v)
       : [_2] "r" (a)
       );

  g(v);
}

#endif

The asm output produced is:

## BUG
#APP
# 23 "bug.c" 1
        mrs %rdi, pmccntr_eli0
        ldr x1, [%rdi]

# 0 "" 2
#NO_APP


## NO_BUG
#APP
# 6 "bug.c" 1
        mrs %rax, pmccntr_eli0
        ldr x1, [%rdi]

# 0 "" 2
#NO_APP

As one can see, in the "NO BUG" case, the registers allocated to reading "the
counter" and to reading "memory", are different, whereas they are one and the
same in the "BUG" case.

This was obtained with today's trunk built for native compilation: gcc (GCC)
14.0.0 20230518 (experimental). 

The example was compiled as:
gcc -S -O3 -Wall -Wextra -o bug.S bug.c 

no diagnostic messages are produced.

Same was also observed with gcc-aarch64-none-linux toolchain 13.2 (from Arm
download page), so "target-related issue" might not be an entirely correct
classification for this.

[Bug target/109903] Register misallocation in hand-crafted asm insn, no diagnostics produced

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109903

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
You need to use an early clobber on the output constraint as documented.
https://gcc.gnu.org/onlinedocs/gcc-13.1.0/gcc/Modifiers.html#index-earlyclobber-operand

[Bug target/109903] Register misallocation in hand-crafted asm insn, no diagnostics produced

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109903

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
That is the inline-asm should be written as:

  asm ("mrs %[_1], pmccntr_eli0\n"
       "\tldr x1, [%[_2]]\n"
       : [_1] "=&r" (v)
       : [_2] "r" (a)
       : "memory");

Note since a is memory address you are writing to you should also add the
memory clobber to the inline-asm.

[Bug target/109903] Register misallocation in hand-crafted asm insn, no diagnostics produced

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109903

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
One more thing, GCC does not look at the inline-asm template except while
outputting the assembly.

[Bug target/109903] Register misallocation in hand-crafted asm insn, no diagnostics produced

[dimitri.gorokhovik at free dot fr]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109903

--- Comment #4 from Dimitri Gorokhovik <dimitri.gorokhovik at free dot fr> ---
Hi Andrew,

I'd agree more with "WONTFIX" here ;-) We are not looking for solution. We want
to spare the same hassle to others.

This asm doesn't write to memory, it doesn't even read any -- 'a' is passed
over in the registers with aarch64 and x64_86.

It is also very hard to see the need for early clobber here ... how comes the
version with return value doesn't need it while the other does? The asm
performs regular register loads. Certainly we are not marking all register
loads with early clobbers are we?

WE ended up moving 'a' to output clause, it feels barely more contorted than
the early-clobber method.

> GCC does not look at the inline-asm template except while outputting the assembly.
Yep, reason why I let it emit ARMv8 insns for the x64_86 target ;-)

[Bug target/109903] Register misallocation in hand-crafted asm insn, no diagnostics produced

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109903

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Dimitri Gorokhovik from comment #4)
> Hi Andrew,
> 
> I'd agree more with "WONTFIX" here ;-) We are not looking for solution. We
> want to spare the same hassle to others.

Why you didn't read the documentation which is documents inline-asm. And there
is no way for GCC to know what you mean.

> 
> This asm doesn't write to memory, it doesn't even read any -- 'a' is passed
> over in the registers with aarch64 and x64_86.

Oh I read the instruction wrong.
> 
> It is also very hard to see the need for early clobber here ... how comes
> the version with return value doesn't need it while the other does? The asm
> performs regular register loads. Certainly we are not marking all register
> loads with early clobbers are we?

The easy and fast rule is simple. If you write to an output operand before read
from an input operand, that output operand needs an early clobber. This is how
I knew what the issue was right away without even looking further into what was
going wrong.

Even the documentation is clear here:
"Means (in a particular alternative) that this operand is an earlyclobber
operand, which is written before the instruction is finished using the input
operands."

The reason why sometimes it works sometimes it does not work is depedent on the
register allocator. In the case of return and first argument registers, well
usually those are the same register so the register allocator wants to
minimalize the number of spills and/or register moves.

> 
> WE ended up moving 'a' to output clause, it feels barely more contorted than
> the early-clobber method.

That is actually WRONG and will produce wrong code in some cases. The early
clobber is the right appoarch here if you are writing to an operand before you
use all of the input operands (again this is all DOCUMENTED and has been for
the over 10 years now).

> 
> > GCC does not look at the inline-asm template except while outputting the assembly.
> Yep, reason why I let it emit ARMv8 insns for the x64_86 target ;-)

[Bug target/109903] Register misallocation in hand-crafted asm insn, no diagnostics produced

[dimitri.gorokhovik at free dot fr]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109903

--- Comment #6 from Dimitri Gorokhovik <dimitri.gorokhovik at free dot fr> ---
Thank you Andrew, I indeed see now the early-clobber effect of this code.

It isn't that we don't read documentation (we do time to time), rather our real
asm statement has more output ops and more elementary instructions in the
template, so the early-clobber effect doesn't jump to the eye quite as easily.
I guess we have to mask all output ops as early clobbers.