Implement ACLE 2.0 macros of chapters 6.6 and 6.7 for [ARM] and [Aarch64] targets

[labrinea]

Summary

Added the remaining macros and corresponding regression tests.

[labrinea]

Ping

[rengolin]

lib/Basic/Targets.cpp

4785 ↗

(On Diff #34029)

I'm not convinced by the runtime rounding being depending on C99, or why it's not possible in freestanding environments.

4790 ↗

(On Diff #34029)

Er, well, the ACLE doc says it wasn't adopted in C11, but that doesn't mean that it was present in all others, just that there is a proposal for C11 which wasn't accepted.

I think this macro needs some other mechanism to be set, and unless Clang/LLVM supports signaling NANs unconditionally, we shouldn't add it.

5270 ↗

(On Diff #34029)

same comments as above.

[labrinea]

lib/Basic/Targets.cpp

4785 ↗

(On Diff #34029)

I copied the same logic from AArch64.

4786 ↗

(On Diff #34029)

Shall we define this unconditionally?

4790 ↗

(On Diff #34029)

Maybe we could skip this macro in this patch? @richard.barton.arm

5270 ↗

(On Diff #34029)

This was present already.

[rengolin]

Adding Richard Smith, as he was the one choosing C99/!freestanding for runtime rounding.

lib/Basic/Targets.cpp

4790 ↗

(On Diff #34029)

If this item sparks controversy, we should keep it out. For now, I'm interested in what Tim and Richard(s) have to say.

[richard.barton.arm]

Hi all

I am uncomfortable with this patch for a number of reasons. These macros seem to me to be defined by the ACLE as describing the behaviour of the combination of library and compiler. For example, the STDC_IEC_599 macro would need some standards compliance from the compiler in terms of optimisations on floating point functions, and from the libraries in terms of the behaviour of the fp functions there. Thus, I don't think it is safe for clang to define this macro for all ARM compilation regardless of which library is being used to link with. Perhaps I have misunderstood that one - so it would be worth running this past Tim directly as the __ARM_FP_FENV_ROUNDING definition comes from his original AArch64 patches from early 2013.

If I am right - then clang would want to adopt an approach similar to GCC's described here https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html (grep for __GCC_IEC_559) where the compiler sets a macro which presumably is read by a compliant c library header which then sets the ACLE macro.

However, there are multiple bug reports in the area of FP math - see https://llvm.org/bugs/show_bug.cgi?id=8100 - which makes me think that for Clang to really be ACLE compliant, we should not be defining any of these macros apart from __ARM_FP_FAST. Additionally a bit of research on the devs list suggest that LLVM does not support signaling NaNs e.g. http://lists.llvm.org/pipermail/llvm-dev/2014-April/071699.html and http://lists.llvm.org/pipermail/llvm-dev/2014-August/075749.html from last year

I suggest that the patch needs to be resubmitted without the setting of ARM_FP_FENV_ROUNDING, STDC_IEC_559 and SUPPORT_SNAN, and that the setting of ARM_FP_FENV_ROUNDING in the aarch64 target be clarified.

[labrinea]

Patch updated.

[labrinea]

@rsmith @t.p.northover could you please give us some feedback on this? Thanks.

[rengolin]

Richard,

The original patch was following a lot from the AArch64, so I suspect we should first fix the AArch64 side, so that we can follow in a similar way. I don't want to have two different behaviour for ARM and AArch64 regarding macros and ACLE support. But I also don't think we should include AArch64 code with this patch.

cheers,
--renato

[richard.barton.arm]

Yes - I would be interested in knowing the rationale behind the original AArch64 setting of __ARM_FP_FPENV_ROUNDING.

I also question whether our pre-conditions for __ARM_FP_FAST are sufficient. There are many -f... options relating to maths operations, are these two the only ones that might not preserve the order of operations? I think clang ought to take a conservative stance when setting these flags.

[rengolin]

In D12633#249966, @richard.barton.arm wrote:

I think clang ought to take a conservative stance when setting these flags.

Exactly my point. This is one of the few examples where being pragmatic may lead to big headaches in the future. :)

[labrinea]

GCC online docs state:

-ffast-math
Might allow some programs designed to not be too dependent on IEEE behavior for floating-point to run faster, or die trying. Sets -funsafe-math-optimizations, -ffinite-math-only, and -fno-trapping-math.

-funsafe-math-optimizations
Allow optimizations that may be give incorrect results for certain IEEE inputs.

-ffinite-math-only
Allow optimizations for floating-point arithmetic that assume that arguments and results are not NaNs or +-Infs.

-fno-trapping-math
Allow the compiler to assume that floating-point arithmetic will not generate traps on any inputs. This is useful, for example, when running a program using IEEE "non-stop" floating-point arithmetic.

Does this mean, define _ARM_FP_FAST when all these three flags are set (-funsafe-math-optimizations, -ffinite-math-only, and -fno-trapping-math)? We are currently checking ("-ffast-math" or "-ffinite-math-only").

ACLE docs state:

__ARM_FP_FAST is defined to 1 if floating-point optimizations may occur such that the computed results are different from those prescribed by the order of operations according to the C standard. Examples of such optimizations would be reassociation of expressions to reduce depth, and replacement of a division by constant with multiplication by its reciprocal.

[t.p.northover]

I can't explain the Opts.C99 (fesetround is provided by conforming C++11 implementations too), but I think I was just trying to capture the fact that decent hosted platforms support fesetround but freestanding ones don't (not having any library).

Supporting "C89 but we also have math.h" (or more realistically "C++03 with math.h") wasn't really something that interested me: users should be encouraged to write to standards.

Cheers.

Tim.

[rengolin]

In D12633#250612, @labrinea wrote:

__ARM_FP_FAST is defined to 1 if floating-point optimizations may occur such that the computed results are different from those prescribed by the order of operations according to the C standard. Examples of such optimizations would be reassociation of expressions to reduce depth, and replacement of a division by constant with multiplication by its reciprocal.

This seems to be specific to the order of operations, which is related to "-funsafe-math-optimizations", but not necessarily the other three. If Clang has, and sets the same flags as GCC docs imply, we should only set the macro for that one flag.

[labrinea]

@t.p.northover I think we should not be defining _ARM_FP_FENV_ROUNDING on neither of ARM and AArch64 targets since "-frounding-math" is not available on clang: clang/llvm don't support C99 FP rounding mode pragmas (FENV_ACCESS etc)

@rengolin Clang translates frontend GCC compliant flags to cc1 flags:

• -fno-honor-infinities => -menable-no-infs
• -fno-honor-nans => -menable-no-nans
• AND( -fassociative-math, -freciprocal-math, -fno-signed-zeros, -fno-trapping-math, -fno-math-errno ) => -menable-unsafe-fp-math

Those flags seem to affect an LLVM IR in two ways:

• function attributes (no-infs-fp-math = yes|no, no-nans-fp-math = yes|no, unsafe-fp-math = yes|no)
• fast-math flags used with generated fp-math operators (this doc summarizes the LLVM-IR fast-math flags)

I think we should not be checking Opts.FiniteMathOnly since it only indicates whether optimizations assume the arguments and result to be +/-inf. We should not be checking Opts.FastMath either since it might be the case where -menable-unsafe-fp-math is set but -ffast-math is not set. Then the user would rely on _ARM_FP_FAST being absent but in reality the backend would emit unsafe fp math IR. My patch introduces a new lang option to handle this case.

[richard.barton.arm]

lib/Frontend/CompilerInvocation.cpp

1711 ↗

(On Diff #35628)

What happens if I do -menable-unsafe-fp-math then -mno-enable-unsafe-fp-math? I don't think you have enough regression tests.

[labrinea]

lib/Frontend/CompilerInvocation.cpp

1711 ↗

(On Diff #35628)

Well there is no such flag (-mno-enable-unsafe-fp-math) but probably what you want to say is that the order of the front-end flags matters. In llvm/tools/clang/test/Driver/fast-math.c there are tests checking whether "-menable-unsafe-fp-math" is correctly set depending on the order of the front-end flags. I could modify my tests so that they pass cc1 flags. Then we would be checking if _ARM_FP_FAST gets defined in the presence of "-menable-unsafe-fp-math". Would that be preferable?

[richard.barton.arm]

In the overall approach, whilst this is the pragmatic approach to get the job done, it clearly takes liberties with the definition of LangOpt as this your new flag is most certainly a CodeGenOpt rather than a LangOpt. I'd like to see what the others say as I don't see a better solution.

lib/Frontend/CompilerInvocation.cpp

1711 ↗

(On Diff #35628)

Ok - I did not realise that by this point in the code we know this has been taken care of. If that argument processing is already covered by tests then there is no need to add more and I retract my suggestion.

[labrinea]

In D12633#252758, @labrinea wrote:

@t.p.northover I think we should not be defining _ARM_FP_FENV_ROUNDING on neither of ARM and AArch64 targets since "-frounding-math" is not available on clang: clang/llvm don't support C99 FP rounding mode pragmas (FENV_ACCESS etc)

@t.p.northover : "Ah, that constant folding is definitely a problem. Changing it sounds reasonable.
Tim."

(attaching Tim's comment since it was missing from the phabricator thread)

[labrinea]

ping

[richard.barton.arm]

What do you think about this Renato? Although Alexandros has added a LangOption which is strictly speaking to do with CodeGen rather than Lang, I think this is the only pragmatica way to do this without needing re-engineering work to expose the codegen options to this phase in clang.

Are you and I able to make the call to let Alexandros commit, or do you think we ought to seek guidance from elsewhere? Do you have an idea who we could go to?

[rengolin]

Sorry, I've been redirected elsewhere for a few days.

I agree that this is the best option, since that's the meaning of the flag, anyway.

LGTM.

--renato

cfe/trunk/include/clang/Basic/LangOptions.def

VALUE_LANGOPT(MaxTypeAlign  , 32, 0,
               "default maximum alignment for types")
 VALUE_LANGOPT(PICLevel    , 2, 0, "__PIC__ level")
 VALUE_LANGOPT(PIELevel    , 2, 0, "__PIE__ level")
 LANGOPT(GNUInline         , 1, 0, "GNU inline semantics")
 COMPATIBLE_LANGOPT(NoInlineDefine    , 1, 0, "__NO_INLINE__ predefined macro")
 COMPATIBLE_LANGOPT(Deprecated        , 1, 0, "__DEPRECATED predefined macro")
 LANGOPT(FastMath          , 1, 0, "__FAST_MATH__ predefined macro")
 LANGOPT(FiniteMathOnly    , 1, 0, "__FINITE_MATH_ONLY__ predefined macro")
+LANGOPT(UnsafeFPMath      , 1, 0, "Unsafe Floating Point Math")
 
 BENIGN_LANGOPT(ObjCGCBitmapPrint , 1, 0, "printing of GC's bitmap layout for __weak/__strong ivars")
 
 BENIGN_LANGOPT(AccessControl     , 1, 1, "C++ access control")
 LANGOPT(CharIsSigned      , 1, 1, "signed char")
 LANGOPT(ShortWChar        , 1, 0, "unsigned short wchar_t")
 ENUM_LANGOPT(MSPointerToMemberRepresentationMethod, PragmaMSPointersToMembersKind, 2, PPTMK_BestCase, "member-pointer representation method")
 

cfe/trunk/lib/Basic/Targets.cpp

void getTargetDefines(const LangOptions &Opts,
     if ((ArchVersion == 6 && CPUProfile != "M") || ArchVersion > 6 ) {
       Builder.defineMacro("__ARM_FEATURE_SAT", "1");
       SAT = true;
     }
 
     // ACLE 6.4.6 Q (saturation) flag
     if (DSP || SAT)
       Builder.defineMacro("__ARM_FEATURE_QBIT", "1");
+
+    if (Opts.UnsafeFPMath)
+      Builder.defineMacro("__ARM_FP_FAST", "1");
   }
 
   void getTargetBuiltins(const Builtin::Info *&Records,
                          unsigned &NumRecords) const override {
     Records = BuiltinInfo;
     NumRecords = clang::ARM::LastTSBuiltin-Builtin::FirstTSBuiltin;
   }
   bool isCLZForZeroUndef() const override { return false; }
void getTargetDefines(const LangOptions &Opts,
     Builder.defineMacro("__LP64__");
 
     // ACLE predefines. Many can only have one possible value on v8 AArch64.
     Builder.defineMacro("__ARM_ACLE", "200");
     Builder.defineMacro("__ARM_ARCH", "8");
     Builder.defineMacro("__ARM_ARCH_PROFILE", "'A'");
 
     Builder.defineMacro("__ARM_64BIT_STATE", "1");
-    Builder.defineMacro("__ARM_PCS_AAPCS64");
+    Builder.defineMacro("__ARM_PCS_AAPCS64", "1");
     Builder.defineMacro("__ARM_ARCH_ISA_A64", "1");
 
     Builder.defineMacro("__ARM_FEATURE_CLZ", "1");
     Builder.defineMacro("__ARM_FEATURE_FMA", "1");
     Builder.defineMacro("__ARM_FEATURE_LDREX", "0xF");
     Builder.defineMacro("__ARM_FEATURE_IDIV", "1"); // As specified in ACLE
     Builder.defineMacro("__ARM_FEATURE_DIV");  // For backwards compatibility
     Builder.defineMacro("__ARM_FEATURE_NUMERIC_MAXMIN", "1");
     Builder.defineMacro("__ARM_FEATURE_DIRECTED_ROUNDING", "1");
 
     Builder.defineMacro("__ARM_ALIGN_MAX_STACK_PWR", "4");
 
     // 0xe implies support for half, single and double precision operations.
     Builder.defineMacro("__ARM_FP", "0xE");
 
     // PCS specifies this for SysV variants, which is all we support. Other ABIs
     // may choose __ARM_FP16_FORMAT_ALTERNATIVE.
     Builder.defineMacro("__ARM_FP16_FORMAT_IEEE", "1");
     Builder.defineMacro("__ARM_FP16_ARGS", "1");
 
-    if (Opts.FastMath || Opts.FiniteMathOnly)
-      Builder.defineMacro("__ARM_FP_FAST");
-
-    if (Opts.C99 && !Opts.Freestanding)
-      Builder.defineMacro("__ARM_FP_FENV_ROUNDING");
+    if (Opts.UnsafeFPMath)
+      Builder.defineMacro("__ARM_FP_FAST", "1");
 
     Builder.defineMacro("__ARM_SIZEOF_WCHAR_T", Opts.ShortWChar ? "2" : "4");
 
     Builder.defineMacro("__ARM_SIZEOF_MINIMAL_ENUM",
                         Opts.ShortEnums ? "1" : "4");
 
     if (FPU == NeonMode) {
       Builder.defineMacro("__ARM_NEON", "1");

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

#include "clang/Frontend/LangStandards.def"
   // inlining enabled.
   Opts.NoInlineDefine = !Opt || Args.hasArg(OPT_fno_inline);
 
   Opts.FastMath = Args.hasArg(OPT_ffast_math) ||
       Args.hasArg(OPT_cl_fast_relaxed_math);
   Opts.FiniteMathOnly = Args.hasArg(OPT_ffinite_math_only) ||
       Args.hasArg(OPT_cl_finite_math_only) ||
       Args.hasArg(OPT_cl_fast_relaxed_math);
+  Opts.UnsafeFPMath = Args.hasArg(OPT_menable_unsafe_fp_math) ||
+                      Args.hasArg(OPT_cl_unsafe_math_optimizations) ||
+                      Args.hasArg(OPT_cl_fast_relaxed_math);
 
   Opts.RetainCommentsFromSystemHeaders =
       Args.hasArg(OPT_fretain_comments_from_system_headers);
 
   unsigned SSP = getLastArgIntValue(Args, OPT_stack_protector, 0, Diags);
   switch (SSP) {
   default:
     Diags.Report(diag::err_drv_invalid_value)

cfe/trunk/test/Preprocessor/aarch64-target-features.c

 // CHECK: __ARM_FEATURE_IDIV 1
 // CHECK: __ARM_FEATURE_LDREX 0xF
 // CHECK: __ARM_FEATURE_NUMERIC_MAXMIN 1
 // CHECK: __ARM_FEATURE_UNALIGNED 1
 // CHECK: __ARM_FP 0xE
 // CHECK: __ARM_FP16_ARGS 1
 // CHECK: __ARM_FP16_FORMAT_IEEE 1
 // CHECK-NOT: __ARM_FP_FAST 1
-// CHECK: __ARM_FP_FENV_ROUNDING 1
 // CHECK: __ARM_NEON 1
 // CHECK: __ARM_NEON_FP 0xE
 // CHECK: __ARM_PCS_AAPCS64 1
+// CHECK-NOT: __ARM_PCS 1
+// CHECK-NOT: __ARM_PCS_VFP 1
 // CHECK-NOT: __ARM_SIZEOF_MINIMAL_ENUM 1
 // CHECK-NOT: __ARM_SIZEOF_WCHAR_T 2
 
 // RUN: %clang -target aarch64_be-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-BIGENDIAN
 // CHECK-BIGENDIAN: __ARM_BIG_ENDIAN 1
 
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8-a+crypto -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRYPTO %s
 // RUN: %clang -target arm64-none-linux-gnu -march=armv8-a+crypto -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRYPTO %s
 // CHECK-CRYPTO: __ARM_FEATURE_CRYPTO 1
 
 // RUN: %clang -target aarch64-none-linux-gnu -mcrc -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRC32 %s
 // RUN: %clang -target arm64-none-linux-gnu -mcrc -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRC32 %s
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8-a+crc -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRC32 %s
 // RUN: %clang -target arm64-none-linux-gnu -march=armv8-a+crc -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-CRC32 %s
 // CHECK-CRC32: __ARM_FEATURE_CRC32 1
 
+// RUN: %clang -target aarch64-none-linux-gnu -fno-math-errno -fno-signed-zeros\
+// RUN:        -fno-trapping-math -fassociative-math -freciprocal-math\
+// RUN:        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
 // RUN: %clang -target aarch64-none-linux-gnu -ffast-math -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
 // RUN: %clang -target arm64-none-linux-gnu -ffast-math -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
 // CHECK-FASTMATH: __ARM_FP_FAST 1
 
 // RUN: %clang -target aarch64-none-linux-gnu -fshort-wchar -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SHORTWCHAR %s
 // RUN: %clang -target arm64-none-linux-gnu -fshort-wchar -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SHORTWCHAR %s
 // CHECK-SHORTWCHAR: __ARM_SIZEOF_WCHAR_T 2
 

cfe/trunk/test/Preprocessor/arm-target-features.c

 // RUN: %clang -target armv8a -mfloat-abi=hard -x c -E -dM %s | FileCheck --check-prefix=CHECK-V8-BAREHF %s
 // CHECK-V8-BAREHF: __ARMEL__ 1
 // CHECK-V8-BAREHF: __ARM_ARCH 8
 // CHECK-V8-BAREHF: __ARM_ARCH_8A__ 1
 // CHECK-V8-BAREHF: __ARM_FEATURE_CRC32 1
 // CHECK-V8-BAREHF: __ARM_FEATURE_DIRECTED_ROUNDING 1
 // CHECK-V8-BAREHF: __ARM_FEATURE_NUMERIC_MAXMIN 1
 // CHECK-V8-BAREHF: __ARM_NEON__ 1
+// CHECK-V8-BAREHF: __ARM_PCS_VFP 1
 // CHECK-V8-BAREHF: __VFP_FP__ 1
 
 // RUN: %clang -target armv8a -mfloat-abi=hard -mfpu=fp-armv8 -x c -E -dM %s | FileCheck --check-prefix=CHECK-V8-BAREHF-FP %s
 // CHECK-V8-BAREHF-FP-NOT: __ARM_NEON__ 1
 // CHECK-V8-BAREHF-FP: __VFP_FP__ 1
 
 // RUN: %clang -target armv8a -mfloat-abi=hard -mfpu=neon-fp-armv8 -x c -E -dM %s | FileCheck --check-prefix=CHECK-V8-BAREHF-NEON-FP %s
 // RUN: %clang -target armv8a -mfloat-abi=hard -mfpu=crypto-neon-fp-armv8 -x c -E -dM %s | FileCheck --check-prefix=CHECK-V8-BAREHF-NEON-FP %s

 
 // RUN: %clang -target armv8a-eabi -x c -E -dM %s -o - | FileCheck --check-prefix=ARMV8A-EABI %s
 // ARMV8A-EABI:#define __ARM_ARCH_EXT_IDIV__ 1
 
 // RUN: %clang -target armv8a-eabi -x c -E -dM %s -o - | FileCheck --check-prefix=THUMBV8A-EABI %s
 // THUMBV8A-EABI:#define __ARM_ARCH_EXT_IDIV__ 1
 
 // RUN: %clang -target arm-none-linux-gnu -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-DEFS %s
+// CHECK-DEFS:#define __ARM_PCS 1
 // CHECK-DEFS:#define __ARM_SIZEOF_MINIMAL_ENUM 4
 // CHECK-DEFS:#define __ARM_SIZEOF_WCHAR_T 4
 
+// RUN: %clang -target arm-none-linux-gnu -fno-math-errno -fno-signed-zeros\
+// RUN:        -fno-trapping-math -fassociative-math -freciprocal-math\
+// RUN:        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
+// RUN: %clang -target arm-none-linux-gnu -ffast-math -x c -E -dM %s -o -\
+// RUN:        | FileCheck --check-prefix=CHECK-FASTMATH %s
+// CHECK-FASTMATH: __ARM_FP_FAST 1
+
 // RUN: %clang -target arm-none-linux-gnu -fshort-wchar -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SHORTWCHAR %s
 // CHECK-SHORTWCHAR:#define __ARM_SIZEOF_WCHAR_T 2
 
 // RUN: %clang -target arm-none-linux-gnu -fshort-enums -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SHORTENUMS %s
 // CHECK-SHORTENUMS:#define __ARM_SIZEOF_MINIMAL_ENUM 1
 
 // Test that -mhwdiv has the right effect for a target CPU which has hwdiv enabled by default.
 // RUN: %clang -target armv7 -mcpu=cortex-a15 -x c -E -dM %s -o - | FileCheck --check-prefix=DEFAULTHWDIV-ARM %s