Setting fp trapping mode and denormal type

[SjoerdMeijer]

Summary

Clang patch D23840 introduced ways to set the FP exceptions and denormal types. This is the LLVM counterpart and it adds options that map onto FP exceptions and denormal build attributes allowing better fp math library selections.

[rengolin]

Overall looks good to me, too.

lib/Target/ARM/ARMAsmPrinter.cpp

460 ↗

(On Diff #69820)

What if we have conflicting attributes?

[jmolloy]

lib/Target/ARM/ARMAsmPrinter.cpp

460 ↗

(On Diff #69820)

Then we are completely, totally, and utterly broken. Both currently and with this patch.

The LTO build attribute emission needs full-on rework and there are several FIXMEs to this effect around the AsmPrinter :(

[jmolloy]

After discussion with Renato on IRC, we're both happy with this.

[echristo]

llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp

454–460

No, this isn't the right place or way to do this.

You are going to want to look at iterating over the functions in a module in order to collect and collate options if you want to set a global asm marker.

[arsenm]

llvm/trunk/include/llvm/CodeGen/CommandFlags.h

158–159 ↗

(On Diff #69850)

Should this be per-FP type? We currently use a subtarget feature for this but need to split between f16/f32/f64

[SjoerdMeijer]

llvm/trunk/include/llvm/CodeGen/CommandFlags.h

158–159 ↗

(On Diff #69850)

I now see how you are using it. We adopted the same approach as some other fp options (no NaNs, no infs, etc), which are also not per FP type. So to be honest, I don't know if they should.

llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp

454–460

Ok, I will fix this.

[arsenm]

llvm/trunk/include/llvm/CodeGen/CommandFlags.h

158–159 ↗

(On Diff #69850)

The other options are quite different from this. They are optimization hints that aren't required for correctness (and also are deprecated in favor of the per-instruction flags). The global denormal handling mode is different and required

[SjoerdMeijer]

llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp

454–460

I have looked at this again and got well confused (this is a messy bit of llvm). But I came to the same conclusion as earlier: it is not more broken than it already was. For example, options NoNaNsFPMath and NoInfsFPMath have the same problem. I think we all agree that the right approach is to "look at iterating over the functions in a module in order to collect and collate options". Do you want to fix this now? I started drafting a patch that fixes this issue for the options I introduced, but I am not happy with it. It does things in a different way than the other options, and I am not sure TargetMachine is the right place (I can upload the patch though if that is helpful).

[SjoerdMeijer]

First attempt to iterate over the functions in a module in order to collect and collate options.

[echristo]

One nit, then OK.

Thanks!

-eric

lib/Target/TargetMachine.cpp

94 ↗

(On Diff #70138)

Bike shed:

No real reason to have this here, just go ahead an have it be a static function in the arm asm printer for now.

[SjoerdMeijer]

Thanks for the review! Will fix and commit.
Cheers.

[SjoerdMeijer]

llvm/trunk/include/llvm/CodeGen/CommandFlags.h

158–159 ↗

(On Diff #69850)

One approach to implement setting denormals per-FP type could be to consider this as the "global flag" that sets all these per-type flags. Then any combination can be made by using the global flag that sets all, or omit it and specify one or more of the per-type flags.

llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp

 }
 
 void ARMAsmPrinter::EmitStartOfAsmFile(Module &M) {
   const Triple &TT = TM.getTargetTriple();
   // Use unified assembler syntax.
   OutStreamer->EmitAssemblerFlag(MCAF_SyntaxUnified);
 
   // Emit ARM Build Attributes
-  if (TT.isOSBinFormatELF()) {
-    if (!M.empty()) {
-      // FIXME: this is a hack, but it is not more broken than
-      // resetTargetOptions already was. The purpose of reading the target
-      // options here is to read function attributes denormal and trapping-math
-      // that we want to map onto build attributes.
-      TM.resetTargetOptions(*M.begin());
-    }
+  if (TT.isOSBinFormatELF())
     emitAttributes();
-  }
 
   // Use the triple's architecture and subarchitecture to determine
   // if we're thumb for the purposes of the top level code16 assembler
   // flag.
   bool isThumb = TT.getArch() == Triple::thumb ||
                  TT.getArch() == Triple::thumbeb ||
                  TT.getSubArch() == Triple::ARMSubArch_v7m ||
                  TT.getSubArch() == Triple::ARMSubArch_v6m;
   if (!M.getModuleInlineAsm().empty() && isThumb)
     OutStreamer->EmitAssemblerFlag(MCAF_Code16);
 }
 
 static void
static ARMBuildAttrs::CPUArch getArchForCPU(StringRef CPU,
   else if (Subtarget->hasV5TOps())
     return ARMBuildAttrs::v5T;
   else if (Subtarget->hasV4TOps())
     return ARMBuildAttrs::v4T;
   else
     return ARMBuildAttrs::v4;
 }
 
+// Returns true if all functions have the same function attribute value
+static bool haveAllFunctionsAttribute(const Module &M, StringRef Attr,
+                                      StringRef Value) {
+  for (auto &F : M)
+    if (F.getFnAttribute(Attr).getValueAsString() != Value)
+      return false;
+
+  return true;
+}
+
+
 void ARMAsmPrinter::emitAttributes() {
   MCTargetStreamer &TS = *OutStreamer->getTargetStreamer();
   ARMTargetStreamer &ATS = static_cast<ARMTargetStreamer &>(TS);
 
   ATS.emitTextAttribute(ARMBuildAttrs::conformance, "2.09");
 
   ATS.switchVendor("aeabi");
 
if (isPositionIndependent()) {
     ATS.emitAttribute(ARMBuildAttrs::ABI_PCS_GOT_use,
                       ARMBuildAttrs::AddressGOT);
   } else {
     ATS.emitAttribute(ARMBuildAttrs::ABI_PCS_GOT_use,
                       ARMBuildAttrs::AddressDirect);
   }
 
   // Set FP Denormals.
-  if (TM.Options.FPDenormalType == FPDenormal::PreserveSign)
-      ATS.emitAttribute(ARMBuildAttrs::ABI_FP_denormal,
-                        ARMBuildAttrs::PreserveFPSign);
-  else if (TM.Options.FPDenormalType == FPDenormal::PositiveZero)
-      ATS.emitAttribute(ARMBuildAttrs::ABI_FP_denormal,
-                        ARMBuildAttrs::PositiveZero);
+  if (haveAllFunctionsAttribute(*MMI->getModule(), "denormal-fp-math",
+                                "preserve-sign") ||
+      TM.Options.FPDenormalType == FPDenormal::PreserveSign)
+  else if (haveAllFunctionsAttribute(*MMI->getModule(), "denormal-fp-math",
+                                     "positive-zero") ||
+           TM.Options.FPDenormalType == FPDenormal::PositiveZero)
   else if (!TM.Options.UnsafeFPMath)
     ATS.emitAttribute(ARMBuildAttrs::ABI_FP_denormal,
                       ARMBuildAttrs::IEEEDenormals);
   else {
     if (!STI.hasVFP2()) {
       // When the target doesn't have an FPU (by design or
       // intention), the assumptions made on the software support
       // mirror that of the equivalent hardware support *if it
else {
     // to whether denormals are flushed to positive zero or to
     // whatever the sign of zero is (ARM v7AR ARM 2.7.5). Historically
     // LLVM has chosen to flush this to positive zero (most likely for
     // GCC compatibility), so that's the chosen value here (the
     // absence of its emission implies zero).
   }
 
   // Set FP exceptions and rounding
-  if (TM.Options.NoTrappingFPMath)
+  if (haveAllFunctionsAttribute(*MMI->getModule(), "no-trapping-math", "true") ||
+      TM.Options.NoTrappingFPMath)
     ATS.emitAttribute(ARMBuildAttrs::ABI_FP_exceptions,
                       ARMBuildAttrs::Not_Allowed);
   else if (!TM.Options.UnsafeFPMath) {
     ATS.emitAttribute(ARMBuildAttrs::ABI_FP_exceptions, ARMBuildAttrs::Allowed);
 
     // If the user has permitted this code to choose the IEEE 754
     // rounding at run-time, emit the rounding attribute.
     if (TM.Options.HonorSignDependentRoundingFPMathOption)