[libc][NFC] Fix GCC inline asm compilation problems + workaround clang assertion

[abrachet]

Summary

The clobber list "cc" is added to inline assembly to workaround a clang assertion that triggers when building with a clang built with assertions enabled. See bug 53391.

The sqrtl input constraint was changed from f to t to fix compilation errors in gcc. Not that it matters much, it's nearly impossible to build with gcc anyway.

See https://godbolt.org/z/z3bc6a9PM showing functionally same output assembly.

[sivachandra]

That a compiler unnecessarily requires the "cc" clobber specification is OK but GCC requiring the input operand to have the "f" modifier instead of the "t" modifier seems not so correct. Should we really care about the GCC problem now?

[abrachet]

In D118099#3268320, @sivachandra wrote:

That a compiler unnecessarily requires the "cc" clobber specification is OK but GCC requiring the input operand to have the "f" modifier instead of the "t" modifier seems not so correct. Should we really care about the GCC problem now?

Ok I've committed without the changes to sqrtl

[mcgrathr]

In D118099#3269740, @abrachet wrote:

In D118099#3268320, @sivachandra wrote:

That a compiler unnecessarily requires the "cc" clobber specification is OK but GCC requiring the input operand to have the "f" modifier instead of the "t" modifier seems not so correct. Should we really care about the GCC problem now?

Ok I've committed without the changes to sqrtl

When something arcane like this is there for a specific reason like working around a compiler bug, then this should always have comments and ideally those comments should have URLs to the compiler bug report so in future it's clear why something is there and when it can be removed. If these instructions don't actually touch the condition codes in hardware, then a proper asm would not say they do. If there's a need to do something other than what the instruction's hardware definition should suggest, it needs a comment. Instructions like fsqrt are documented in the Intel manual to touch the FPU status flags. Perhaps GCC is using the "cc" clobber to also indicate the condition code-like portions of the FPU status register. (However, I don't see any indications of this in GCC's source where it emits fsqrt itself.)

In the case of "f" vs "t" I think this is a misuse of the constraint semantics, which are specified by GCC. The "t" constraint for x86 is a specific register, not a class of registers. In GCC, when two operands must actually be the same thing exactly then this must be specified directly by using a matching constraint (just digits). So here, you need to use "=t" in the output and then "0" in the input to say that the input operand must be identical to the output operand. Otherwise you're saying they should be two separate operands that both meet the "t" constraint. Since that constraint is only met by a single register, that's impossible. Using the "f" constraint tells it to use some other x87 register, which won't be the actual input to the machine instruction.

[sivachandra]

In D118099#3270766, @mcgrathr wrote:

In the case of "f" vs "t" I think this is a misuse of the constraint semantics, which are specified by GCC. The "t" constraint for x86 is a specific register, not a class of registers. In GCC, when two operands must actually be the same thing exactly then this must be specified directly by using a matching constraint (just digits). So here, you need to use "=t" in the output and then "0" in the input to say that the input operand must be identical to the output operand. Otherwise you're saying they should be two separate operands that both meet the "t" constraint. Since that constraint is only met by a single register, that's impossible. Using the "f" constraint tells it to use some other x87 register, which won't be the actual input to the machine instruction.

Thanks for explaining. I verified that using the "0" modifier for the input argument fixes the GCC problem. Without that, the error GCC was giving was:

error: output operand 0 must use '&' constraint
    3 |     __asm__ __volatile__("fsqrt" : "=t"(out) : "t"(in));

Which got me to add =& constraint to the output operand and GCC promptly complained about the "impossible constraint" that you have explained.

[mcgrathr]

I think in fact the proper thing for this case is to just use a single operand "+t" to directly indicate that the one operand is both input and output. I'm not sure that "=&t" and "0" will behave any different, but this seems to be exactly what "+" is for.

[sivachandra]

In D118099#3270812, @mcgrathr wrote:

I think in fact the proper thing for this case is to just use a single operand "+t" to directly indicate that the one operand is both input and output. I'm not sure that "=&t" and "0" will behave any different, but this seems to be exactly what "+" is for.

Indeed, thanks!

libc/src/math/x86_64/cos.cpp

 //===-- Implementation of the cos function for x86_64 ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/math/cos.h"
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(double, cos, (double x)) {
   double result;
-  __asm__ __volatile__("fcos" : "=t"(result) : "f"(x));
+  __asm__ __volatile__("fcos" : "=t"(result) : "f"(x) : "cc");
   return result;
 }
 
 } // namespace __llvm_libc

libc/src/math/x86_64/sin.cpp

 //===-- Implementation of the sin function for x86_64 ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/math/sin.h"
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(double, sin, (double x)) {
   double result;
-  __asm__ __volatile__("fsin" : "=t"(result) : "f"(x));
+  __asm__ __volatile__("fsin" : "=t"(result) : "f"(x) : "cc");
   return result;
 }
 
 } // namespace __llvm_libc

libc/src/math/x86_64/tan.cpp

 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(double, tan, (double x)) {
   double result;
   double one;
   // The fptan instruction pushes the number 1 on to the FP stack after
   // computing tan. So, we read out the one before popping the actual result.
-  __asm__ __volatile__("fptan" : "=t"(one) : "f"(x));
+  __asm__ __volatile__("fptan" : "=t"(one) : "f"(x) : "cc");
   __asm__ __volatile__("fstpl %0" : "=m"(result));
   return result;
 }
 
 } // namespace __llvm_libc