Additionally set f32 mode with denormal-fp-math

[dcandler]

Summary

When the denormal-fp-math option is used, this should set the
denormal handling mode for all floating point types. However,
currently 32-bit float types can ignore this setting as there is a
variant of the option, denormal-fp-math-f32, specifically for that type
which takes priority when checking the mode based on type and remains
at the default of IEEE. From the description, denormal-fp-math would
be expected to set the mode for floats unless overridden by the f32
variant, and code in the front end only emits the f32 option if it is
different to the general one, so setting just denormal-fp-math should
be valid.

This patch changes the denormal-fp-math option to also set the f32
mode. If denormal-fp-math-f32 is also specified, this is then
overridden as expected, but if it is absent floats will be set to the
mode specified by the former option, rather than remain on the default.

[arsenm]

Testcase?

[dcandler]

Added a test that checks attributes based on the -fdenormal-fp-math and -fdenormal-fp-math-f32 flags.

Only the cases where -fdenormal-fp-math is set to preserve-sign or positive-zero and -fdenormal-fp-math-f32 is unset are changed by this patch. Previously, these would still result in a "denormal-fp-math-f32"="ieee,ieee" attribute.

[arsenm]

I think my intent here was to not emit denormal-fp-math-f32 at all if it wasn't specified (i.e. denormal-fp-math-f32 is a special override only really used for AMDGPU)

The interaction between the two and the default is supposed to be handled by the backend interpretation of the attributes. Does this fix an observable problem for you?

[dcandler]

The issue I found was trying to use getDefaultDenormalModeForType during constant folding to account for denormals (https://reviews.llvm.org/D116952). Setting denormal-fp-math to a non-IEEE mode without specifying denormal-fp-math-f32 still results in the denormal-fp-math-f32 attribute being present (even if unsued elsewhere), which leads to the wrong result for targets that do not support denormal-fp-math-f32.

Only emitting denormal-fp-math-f32 when specified makes sense, but the current default effectively always specifies it as IEEE. One alternative would be to simply change the default.

[arsenm]

LGTM. I played around with this and it seems to have the overriding behavior I would expect

clang/test/CodeGen/denormalfpmode-f32.c

22

This CHECK-LABEL is broken since you don't use CHECK, but it also isn't really necessary with only one function

clang/lib/Driver/ToolChains/Clang.cpp

for (const Arg *A : Args) {
 
     case options::OPT_fno_rounding_math:
       RoundingFPMath = false;
       RoundingMathPresent = false;
       break;
 
     case options::OPT_fdenormal_fp_math_EQ:
       DenormalFPMath = llvm::parseDenormalFPAttribute(A->getValue());
+      DenormalFP32Math = DenormalFPMath;
       if (!DenormalFPMath.isValid()) {
         D.Diag(diag::err_drv_invalid_value)
             << A->getAsString(Args) << A->getValue();
       }
       break;
 
     case options::OPT_fdenormal_fp_math_f32_EQ:
       DenormalFP32Math = llvm::parseDenormalFPAttribute(A->getValue());

clang/lib/Frontend/CompilerInvocation.cpp

#undef CODEGEN_OPTION_WITH_MARSHALLING
   if (Opts.ExplicitEmulatedTLS) {
     GenerateArg(
         Args, Opts.EmulatedTLS ? OPT_femulated_tls : OPT_fno_emulated_tls, SA);
   }
 
   if (Opts.FPDenormalMode != llvm::DenormalMode::getIEEE())
     GenerateArg(Args, OPT_fdenormal_fp_math_EQ, Opts.FPDenormalMode.str(), SA);
 
-  if (Opts.FP32DenormalMode != llvm::DenormalMode::getIEEE())
+  if ((Opts.FPDenormalMode != Opts.FP32DenormalMode) ||
+      (Opts.FP32DenormalMode != llvm::DenormalMode::getIEEE()))
     GenerateArg(Args, OPT_fdenormal_fp_math_f32_EQ, Opts.FP32DenormalMode.str(),
                 SA);
 
   if (Opts.StructReturnConvention == CodeGenOptions::SRCK_OnStack) {
     OptSpecifier Opt =
         T.isPPC32() ? OPT_maix_struct_return : OPT_fpcc_struct_return;
     GenerateArg(Args, Opt, SA);
   } else if (Opts.StructReturnConvention == CodeGenOptions::SRCK_InRegs) {
if (T.isOSAIX()) {
       if (Name != "global-dynamic")
         Diags.Report(diag::err_aix_unsupported_tls_model) << Name;
     }
   }
 
   if (Arg *A = Args.getLastArg(OPT_fdenormal_fp_math_EQ)) {
     StringRef Val = A->getValue();
     Opts.FPDenormalMode = llvm::parseDenormalFPAttribute(Val);
+    Opts.FP32DenormalMode = Opts.FPDenormalMode;
     if (!Opts.FPDenormalMode.isValid())
       Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
   if (Arg *A = Args.getLastArg(OPT_fdenormal_fp_math_f32_EQ)) {
     StringRef Val = A->getValue();
     Opts.FP32DenormalMode = llvm::parseDenormalFPAttribute(Val);
     if (!Opts.FP32DenormalMode.isValid())

clang/test/CodeGen/denormalfpmode-f32.c

+// RUN: %clang_cc1 -S %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-NONE,CHECK-F32-NONE
+// RUN: %clang_cc1 -S -fdenormal-fp-math=ieee %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-NONE,CHECK-F32-NONE
+// RUN: %clang_cc1 -S -fdenormal-fp-math=preserve-sign %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-PS,CHECK-F32-NONE
+// RUN: %clang_cc1 -S -fdenormal-fp-math=positive-zero %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-PZ,CHECK-F32-NONE
+
+// RUN: %clang_cc1 -S -fdenormal-fp-math-f32=ieee %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-NONE,CHECK-F32-NONE
+// RUN: %clang_cc1 -S -fdenormal-fp-math=ieee -fdenormal-fp-math-f32=ieee %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-NONE,CHECK-F32-NONE
+// RUN: %clang_cc1 -S -fdenormal-fp-math=preserve-sign -fdenormal-fp-math-f32=ieee %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-PS,CHECK-F32-IEEE
+// RUN: %clang_cc1 -S -fdenormal-fp-math=positive-zero -fdenormal-fp-math-f32=ieee %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-PZ,CHECK-F32-IEEE
+
+// RUN: %clang_cc1 -S -fdenormal-fp-math-f32=preserve-sign %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-NONE,CHECK-F32-PS
+// RUN: %clang_cc1 -S -fdenormal-fp-math=ieee -fdenormal-fp-math-f32=preserve-sign %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-NONE,CHECK-F32-PS
+// RUN: %clang_cc1 -S -fdenormal-fp-math=preserve-sign -fdenormal-fp-math-f32=preserve-sign %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-PS,CHECK-F32-NONE
+// RUN: %clang_cc1 -S -fdenormal-fp-math=positive-zero -fdenormal-fp-math-f32=preserve-sign %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-PZ,CHECK-F32-PS
+
+// RUN: %clang_cc1 -S -fdenormal-fp-math-f32=positive-zero %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-NONE,CHECK-F32-PZ
+// RUN: %clang_cc1 -S -fdenormal-fp-math=ieee -fdenormal-fp-math-f32=positive-zero %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-NONE,CHECK-F32-PZ
+// RUN: %clang_cc1 -S -fdenormal-fp-math=preserve-sign -fdenormal-fp-math-f32=positive-zero %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-PS,CHECK-F32-PZ
+// RUN: %clang_cc1 -S -fdenormal-fp-math=positive-zero -fdenormal-fp-math-f32=positive-zero %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK-ATTR,CHECK-PZ,CHECK-F32-NONE
+
+// CHECK-LABEL: main
+
+// CHECK-ATTR: attributes #0 =
+// CHECK-NONE-NOT:"denormal-fp-math"
+// CHECK-IEEE: "denormal-fp-math"="ieee,ieee"
+// CHECK-PS: "denormal-fp-math"="preserve-sign,preserve-sign"
+// CHECK-PZ: "denormal-fp-math"="positive-zero,positive-zero"
+// CHECK-F32-NONE-NOT:"denormal-fp-math-f32"
+// CHECK-F32-IEEE: "denormal-fp-math-f32"="ieee,ieee"
+// CHECK-F32-PS: "denormal-fp-math-f32"="preserve-sign,preserve-sign"
+// CHECK-F32-PZ: "denormal-fp-math-f32"="positive-zero,positive-zero"
+
+int main(void) {
+  return 0;
+}