fix for not copying fp denormal and trapping options.

[SjoerdMeijer]

Summary

This is a fix for not copying fp denormal and trapping options, and depends on the nfc cleanups in D24906 and D24907.

[SjoerdMeijer]

Ping

[hfinkel]

I'm fine with setting these for consistency, but I don't understand why our failure to do this would cause problems. If you look in lib/CodeGen/CGCall.cpp and you'll see:

if (!CodeGenOpts.FPDenormalMode.empty())
  FuncAttrs.addAttribute("denormal-fp-math",
                         CodeGenOpts.FPDenormalMode);

FuncAttrs.addAttribute("no-trapping-math",
                       llvm::toStringRef(CodeGenOpts.NoTrappingMath));
FuncAttrs.addAttribute("no-infs-fp-math",

and the code in TargetMachine::resetTargetOptions in Target/TargetMachine.cpp has this:

RESET_OPTION(NoTrappingFPMath, "no-trapping-math");

StringRef Denormal =
  F.getFnAttribute("denormal-fp-math").getValueAsString();
if (Denormal == "ieee")
  Options.FPDenormalType = FPDenormal::IEEE;
else if (Denormal == "preserve-sign")
  Options.FPDenormalType = FPDenormal::PreserveSign;
else if (Denormal == "positive-zero")
  Options.FPDenormalType = FPDenormal::PositiveZero;

so this should all work regardless.

Also, please post your patches with full context. Please see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface for instructions.

[SjoerdMeijer]

Hi Hal,
Thanks for reviewing and you're right: this should work. We actually have actually some downstream (aarch64) build attribute selection code that would work better with this change. Are you okay with committing this change?
Cheers,
Sjoerd.

[hfinkel]

In D24909#559110, @SjoerdMeijer wrote:

Hi Hal,
Thanks for reviewing and you're right: this should work. We actually have actually some downstream (aarch64) build attribute selection code that would work better with this change. Are you okay with committing this change?
Cheers,
Sjoerd.

Can you explain what you mean? What does this code do?

My general impression is that we're planning to rip out this code entirely and reply only on setting the functions attributes. @echristo, am I right about this?

[echristo]

Yes, none of this should be added or expanded in TargetOptions.

Thanks!

-eric

[SjoerdMeijer]

Ok, thanks for the feedback and looking into this. I will abandon this change.
To still answer your question, there is nothing more to it than emitting an AArch64 build attribute symbol (for library selection). This done based on function attributes and options. But I've now noticed that this (downstream) logic needs updating. Thanks again.

lib/CodeGen/BackendUtil.cpp

       .Case("posix", llvm::ThreadModel::POSIX)
       .Case("single", llvm::ThreadModel::Single);
 
+  Options.FPDenormalMode =
+    llvm::StringSwitch<llvm::FPDenormal::DenormalMode>(CodeGenOpts.FPDenormalMode)
+      .Case("ieee", llvm::FPDenormal::IEEE)
+      .Case("preserve-sign", llvm::FPDenormal::PreserveSign)
+      .Case("positive-zero", llvm::FPDenormal::PositiveZero)
+      .Default(llvm::FPDenormal::IEEE);
+
   // Set float ABI type.
   assert((CodeGenOpts.FloatABI == "soft" || CodeGenOpts.FloatABI == "softfp" ||
           CodeGenOpts.FloatABI == "hard" || CodeGenOpts.FloatABI.empty()) &&

   Options.LessPreciseFPMADOption = CodeGenOpts.LessPreciseFPMAD;
   Options.NoInfsFPMath = CodeGenOpts.NoInfsFPMath;
   Options.NoNaNsFPMath = CodeGenOpts.NoNaNsFPMath;
+  Options.NoTrappingFPMath = CodeGenOpts.NoTrappingMath;
   Options.NoZerosInBSS = CodeGenOpts.NoZeroInitializedInBSS;
   Options.UnsafeFPMath = CodeGenOpts.UnsafeFPMath;
   Options.StackAlignmentOverride = CodeGenOpts.StackAlignment;