Work on cleaning up denormal mode handling

[arsenm]

Summary

Cleanup handling of the denormal-fp-math attribute. Consolidate places
checking the allowed names in one place. Also begin migrating towards
the current IEEE-754's standard's preferred terminology of subnormal
over denormal.

This is in preparation for introducing FP type specific variants of
the denorm-fp-mode attribute. AMDGPU will switch to using this in
place of the current hacky use of subtarget features for the denormal
mode.

Introduce a new header for dealing with FP modes. The constrained
intrinsic classes define related enums that should also be moved into
this header for uses in other contexts.

The verifier could use a check to make sure the denorm-fp-mode
attribute is sane, but there currently isn't one.

There is a problem with this patch as is. The one place currently
checking this attribute in buildSqrtEstimateImpl (added by D42323)
oddly doesn't assume the ieee behavior if the attribute isn't
specified as I would expect. The question of why this behaves this way
needs to be resolved to proceed as tests fail due to assuming IEEE
behavior on undecorated functions.

[arsenm]

Defer any behavior changes until a future patch, so all tests now pass

[spatel]

clang/lib/CodeGen/CGCall.cpp

1745–1746

Do you plan to change the attribute string from "denormal" to "subnormal" as part of upgrading it to work per-FP-type? Would we need to auto-upgrade old IR as part of making the string consistent with the code?

Can we stash the attribute string name inside a getter function in the new ADT file, so clang and LLVM have a common source of truth for the attribute name?

[arsenm]

clang/lib/CodeGen/CGCall.cpp

1745–1746

I'm considering it, but at the moment I'm trying to avoid changes. The next step I'm working on is adding denormal-fp-math-f32 (or maybe subnormal-fp-math-f32), which will co-exist and override the current attribute if the type matches

[spatel]

clang/lib/CodeGen/CGCall.cpp

1745–1746

I think it would be better to not change the vocabulary incrementally then. Ie, keep everything "denormal" in this patch, and then universally change the terminology to "subnormal" in one step. That way we won't have any inconsistency/confusion between the attribute name and the code.

[arsenm]

clang/lib/CodeGen/CGCall.cpp

1745–1746

In the follow up patch, the new attribute uses the old denormal name. The clang option handling maintains the old name to match the flag, but the new internal enums and functions use the subnormal name. Is that a reasonable state? I don’t want to spread the old name through the new utilities, but also don’t want to have to auto upgrade bitcode for a name change

[spatel]

clang/lib/CodeGen/CGCall.cpp

1745–1746

I'm not seeing the value in using "subnormal" relative to the confusion cost then. If we're always going to use the "denormal" flag in clang, then I'd prefer to have the code be consistent with that name. That's what I'd grep for, so I think that's what anyone viewing the code for the first time would expect too.

[arsenm]

Rename to denormal

[arsenm]

clang/lib/CodeGen/CGCall.cpp

1745–1746

I thought it might be good to move towards the current standard terminology, but it's not critical

[arsenm]

Missed a spot to rename

[spatel]

LGTM - see inline for some leftover naming diffs.

clang/lib/CodeGen/CGCall.cpp

1744–1745

If I'm seeing it correctly, this can't build as-is? FPSubnormalMode is called FPDenormalMode in the header change above here.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

20469

SubnormMode -> DenormMode

[andrew.w.kaylor]

I'm unclear as to the expectations surrounding this option. I suppose this is somewhat beyond the scope of the current changes, but I'm confused by clang's current behavior with regard to denormals.

The -fdenromal-fp-math option causes a function attribute to be set indicating the desired flushing behavior, and I guess in some cases that has an effect on instruction selection, but it seems to be orthogonal to whether or not we're actually setting the processor controls to do flushing (at least for most targets). I really only know what happens in the x86 case, and I don't know if this behavior is consistent across architectures, but in the x86 case setting or not setting the processor control flags depends on the fast math flags and whether or not we find crtfastmath.o when we link.

This leads me to my other point of confusion. Setting the "denormal-fp-math" option on a per-function basis seems wrong for targets that have a global FP control.

clang/include/clang/Basic/CodeGenOptions.h

167

Why is "Invalid" the default here? If you don't use the "fdenormal-fp-math" option, shouldn't you get IEEE?

[arsenm]

In D69598#1739655, @andrew.w.kaylor wrote:

I'm unclear as to the expectations surrounding this option. I suppose this is somewhat beyond the scope of the current changes, but I'm confused by clang's current behavior with regard to denormals.

Yes, the current usage is underspecified and broken by default. I complained about this in this post: http://lists.llvm.org/pipermail/llvm-dev/2019-November/136449.html
The difference between whether input denormals are implicitly flushed and whether denormal results are flushed to zero does matter in the current codegen use.

The -fdenromal-fp-math option causes a function attribute to be set indicating the desired flushing behavior, and I guess in some cases that has an effect on instruction selection, but it seems to be orthogonal to whether or not we're actually setting the processor controls to do flushing (at least for most targets). I really only know what happens in the x86 case, and I don't know if this behavior is consistent across architectures, but in the x86 case setting or not setting the processor control flags depends on the fast math flags and whether or not we find crtfastmath.o when we link.

The current user needs to know if it can safely ignore a denormal input to avoid miscompiling sqrt. For AMDGPU this changes some lowering and instructions that are safely selectable. I would also like to be able possibly use this for constant folding llvm.canonicalize, although I'm unsure if we need a "may flush" or "must flush" distinction.

This leads me to my other point of confusion. Setting the "denormal-fp-math" option on a per-function basis seems wrong for targets that have a global FP control.

That's really going to be all targets. For AMDGPU we can directly set the FP mode for the kernels/entry points from this attribute, but not an arbitrary callable function. I do think the floating point environment bits should be a considered a property of the calling convention, with attributes that override them. A function which calls a function with a different mode would be responsible for switching the mode before the call. This would require people actually caring about getting this right to really implement

clang/include/clang/Basic/CodeGenOptions.h

167

Because the current users are broken, and this minimizes changes in the cleanup patches. The follow up patches fix this and switch the default

[andrew.w.kaylor]

Thanks. I understand your direction for denormal handling now, and I'm OK with this patch apart from the remaining references to subnormal that Sanjay mentioned.

In D69598#1739723, @arsenm wrote:

I do think the floating point environment bits should be a considered a property of the calling convention, with attributes that override them. A function which calls a function with a different mode would be responsible for switching the mode before the call. This would require people actually caring about getting this right to really implement

Do you mean the compiler should insert code to restore the FP environment on function transitions? Or do you mean that the function itself (i.e. the user's code) is responsible for switching the mode? I have some reservations about this, but I think the C standard specification for FENV_ACCESS is clear that the it is the programmer's responsibility to manage the floating point environment correctly. Yes, that's a sure recipe for broken code, but that's what it says. Obviously LLVM IR is not bound by the C standard and we could take a different approach, but I have concerns about the performance implications because in general the compiler won't know when the environment needs to be restored so it would have to take a conservative approach.

I've been meaning to write some documentation on the expected behavior at function boundaries of the FP environment. Perhaps we can continue this discussion there.

[arsenm]

In D69598#1742740, @andrew.w.kaylor wrote:

Thanks. I understand your direction for denormal handling now, and I'm OK with this patch apart from the remaining references to subnormal that Sanjay mentioned.

In D69598#1739723, @arsenm wrote:

I do think the floating point environment bits should be a considered a property of the calling convention, with attributes that override them. A function which calls a function with a different mode would be responsible for switching the mode before the call. This would require people actually caring about getting this right to really implement

Do you mean the compiler should insert code to restore the FP environment on function transitions?

When calling a convention with a different FP mode, yes. For example graphics shaders have a different default FP mode than compute functions. Theoretically a graphics shader could link a compute function library, which would require switching the mode around the call. My consideration isn't really for users using FENV_ACCESS

[arsenm]

7fe9435dc88050ee78eb1d4adec87610dce468f7

This does now need to be merged with the FPEnv.h header

clang/include/clang/Basic/CodeGenOptions.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_BASIC_CODEGENOPTIONS_H
 #define LLVM_CLANG_BASIC_CODEGENOPTIONS_H
 
 #include "clang/Basic/DebugInfoOptions.h"
 #include "clang/Basic/Sanitizers.h"
 #include "clang/Basic/XRayInstr.h"
+#include "llvm/ADT/FloatingPointMode.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Target/TargetOptions.h"
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
public:
   std::string RecordCommandLine;
 
   std::map<std::string, std::string> DebugPrefixMap;
 
   /// The ABI to use for passing floating point arguments.
   std::string FloatABI;
 
   /// The floating-point denormal mode to use.
-  std::string FPDenormalMode;
+  llvm::DenormalMode FPDenormalMode = llvm::DenormalMode::Invalid;
 
   /// The float precision limit to use, if non-empty.
   std::string LimitFloatPrecision;
 
   struct BitcodeFileToLink {
     /// The filename of the bitcode file to link in.
     std::string Filename;
     /// If true, we set attributes functions in the bitcode library according to

clang/lib/CodeGen/CGCall.cpp

if (AttrOnCallSite) {
     }
     FuncAttrs.addAttribute("frame-pointer", FpKind);
 
     FuncAttrs.addAttribute("less-precise-fpmad",
                            llvm::toStringRef(CodeGenOpts.LessPreciseFPMAD));
 
     if (CodeGenOpts.NullPointerIsValid)
       FuncAttrs.addAttribute("null-pointer-is-valid", "true");
-    if (!CodeGenOpts.FPDenormalMode.empty())
-      FuncAttrs.addAttribute("denormal-fp-math", CodeGenOpts.FPDenormalMode);
+    if (CodeGenOpts.FPSubnormalMode != llvm::SubnormalMode::Invalid)
+      FuncAttrs.addAttribute("denormal-fp-math",
+                             llvm::subnormalModeName(CodeGenOpts.FPSubnormalMode));
 
     FuncAttrs.addAttribute("no-trapping-math",
                            llvm::toStringRef(CodeGenOpts.NoTrappingMath));
 
     // Strict (compliant) code is the default, so only add this attribute to
     // indicate that we are trying to workaround a problem case.
     if (!CodeGenOpts.StrictFloatCastOverflow)
       FuncAttrs.addAttribute("strict-float-cast-overflow", "false");

clang/lib/Frontend/CompilerInvocation.cpp

if (Model == ~0U) {
       Success = false;
     } else {
       Opts.setDefaultTLSModel(static_cast<CodeGenOptions::TLSModel>(Model));
     }
   }
 
   if (Arg *A = Args.getLastArg(OPT_fdenormal_fp_math_EQ)) {
     StringRef Val = A->getValue();
-    if (Val == "ieee")
-      Opts.FPDenormalMode = "ieee";
-    else if (Val == "preserve-sign")
-      Opts.FPDenormalMode = "preserve-sign";
-    else if (Val == "positive-zero")
-      Opts.FPDenormalMode = "positive-zero";
-    else
+    Opts.FPDenormalMode = llvm::parseDenormalFPAttribute(Val);
+    if (Opts.FPDenormalMode == llvm::DenormalMode::Invalid)
       Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
   if (Arg *A = Args.getLastArg(OPT_fpcc_struct_return, OPT_freg_struct_return)) {
     if (A->getOption().matches(OPT_fpcc_struct_return)) {
       Opts.setStructReturnConvention(CodeGenOptions::SRCK_OnStack);
     } else {
       assert(A->getOption().matches(OPT_freg_struct_return));

llvm/include/llvm/ADT/FloatingPointMode.h

+//===- llvm/Support/FloatingPointMode.h -------------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+// Utilities for dealing with flags related to floating point mode controls.
+//
+//===----------------------------------------------------------------------===/
+
+#ifndef LLVM_FLOATINGPOINTMODE_H
+#define LLVM_FLOATINGPOINTMODE_H
+
+#include "llvm/ADT/StringSwitch.h"
+
+namespace llvm {
+
+/// Represent handled modes for subnormal (aka denormal) modes in the floating
+/// point environment.
+enum class DenormalMode {
+  Invalid = -1,
+
+  /// IEEE-754 subnormal numbers preserved.
+  IEEE,
+
+  /// The sign of a flushed-to-zero number is preserved in the sign of 0
+  PreserveSign,
+
+  /// Denormals are flushed to positive zero.
+  PositiveZero
+};
+
+/// Parse the expected names from the denormal-fp-math attribute.
+inline DenormalMode parseDenormalFPAttribute(StringRef Str) {
+  // Assume ieee on unspecified attribute.
+  return StringSwitch<DenormalMode>(Str)
+    .Cases("", "ieee", DenormalMode::IEEE)
+    .Case("preserve-sign", DenormalMode::PreserveSign)
+    .Case("positive-zero", DenormalMode::PositiveZero)
+    .Default(DenormalMode::Invalid);
+}
+
+/// Return the name used for the subnormal handling mode used by the the
+/// expected names from the denormal-fp-math attribute.
+inline StringRef denormalModeName(DenormalMode Mode) {
+  switch (Mode) {
+  case DenormalMode::IEEE:
+    return "ieee";
+  case DenormalMode::PreserveSign:
+    return "preserve-sign";
+  case DenormalMode::PositiveZero:
+    return "positive-zero";
+  default:
+    return "";
+  }
+}
+
+}
+
+#endif // LLVM_FLOATINGPOINTMODE_H

llvm/include/llvm/CodeGen/MachineFunction.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_MACHINEFUNCTION_H
 #define LLVM_CODEGEN_MACHINEFUNCTION_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/FloatingPointMode.h"
 #include "llvm/ADT/GraphTraits.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/ilist.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
Ty *getInfo() {
     return static_cast<Ty*>(MFInfo);
   }
 
   template<typename Ty>
   const Ty *getInfo() const {
      return const_cast<MachineFunction*>(this)->getInfo<Ty>();
   }
 
+  /// Returns the subnormal handling type for the default rounding mode of the
+  /// function.
+  DenormalMode getDenormalMode(const fltSemantics &FPType) const;
+
   /// getBlockNumbered - MachineBasicBlocks are automatically numbered when they
   /// are inserted into the machine function.  The block number for a machine
   /// basic block can be found by using the MBB::getNumber method, this method
   /// provides the inverse mapping.
   MachineBasicBlock *getBlockNumbered(unsigned N) const {
     assert(N < MBBNumbering.size() && "Illegal block number");
     assert(MBBNumbering[N] && "Block was removed from the machine function!");
     return MBBNumbering[N];

llvm/include/llvm/CodeGen/SelectionDAG.h

public:
   /// Return the HeapAllocSite type associated with the SDNode, if it exists.
   MDNode *getHeapAllocSite(const SDNode *Node) {
     auto It = SDCallSiteDbgInfo.find(Node);
     if (It == SDCallSiteDbgInfo.end())
       return nullptr;
     return It->second.HeapAllocSite;
   }
 
+  /// Return the current function's default subnormal handling kind for the
+  /// given floating point type.
+  DenormalMode getDenormalMode(EVT VT) const {
+    return MF->getDenormalMode(EVTToAPFloatSemantics(VT));
+  }
+
 private:
   void InsertNode(SDNode *N);
   bool RemoveNodeFromCSEMaps(SDNode *N);
   void AddModifiedNodeToCSEMaps(SDNode *N);
   SDNode *FindModifiedNodeSlot(SDNode *N, SDValue Op, void *&InsertPos);
   SDNode *FindModifiedNodeSlot(SDNode *N, SDValue Op1, SDValue Op2,
                                void *&InsertPos);
   SDNode *FindModifiedNodeSlot(SDNode *N, ArrayRef<SDValue> Ops,

llvm/lib/CodeGen/MachineFunction.cpp

 getOrCreateJumpTableInfo(unsigned EntryKind) {
   if (JumpTableInfo) return JumpTableInfo;
 
   JumpTableInfo = new (Allocator)
     MachineJumpTableInfo((MachineJumpTableInfo::JTEntryKind)EntryKind);
   return JumpTableInfo;
 }
 
+DenormalMode MachineFunction::getDenormalMode(const fltSemantics &FPType) const {
+  // TODO: Should probably avoid the connection to the IR and store directly
+  // in the MachineFunction.
+  Attribute Attr = F.getFnAttribute("denormal-fp-math");
+
+  // FIXME: This should assume IEEE behavior on an unspecified
+  // attribute. However, the one current user incorrectly assumes a non-IEEE
+  // target by default.
+  StringRef Val = Attr.getValueAsString();
+  if (Val.empty())
+    return DenormalMode::Invalid;
+
+  return parseDenormalFPAttribute(Val);
+}
+
 /// Should we be emitting segmented stack stuff for the function
 bool MachineFunction::shouldSplitStack() const {
   return getFunction().hasFnAttribute("split-stack");
 }
 
 LLVM_NODISCARD unsigned
 MachineFunction::addFrameInst(const MCCFIInstruction &Inst) {
   FrameInstructions.push_back(Inst);

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (Iterations) {
             : buildSqrtNRTwoConst(Op, Est, Iterations, Flags, Reciprocal);
 
       if (!Reciprocal) {
         // The estimate is now completely wrong if the input was exactly 0.0 or
         // possibly a denormal. Force the answer to 0.0 for those cases.
         SDLoc DL(Op);
         EVT CCVT = getSetCCResultType(VT);
         ISD::NodeType SelOpcode = VT.isVector() ? ISD::VSELECT : ISD::SELECT;
-        const Function &F = DAG.getMachineFunction().getFunction();
-        Attribute Denorms = F.getFnAttribute("denormal-fp-math");
-        if (Denorms.getValueAsString().equals("ieee")) {
+        DenormalMode SubnormMode = DAG.getDenormalMode(VT);
+        if (SubnormMode == DenormalMode::IEEE) {
           // fabs(X) < SmallestNormal ? 0.0 : Est
           const fltSemantics &FltSem = DAG.EVTToAPFloatSemantics(VT);
           APFloat SmallestNorm = APFloat::getSmallestNormalized(FltSem);
           SDValue NormC = DAG.getConstantFP(SmallestNorm, DL, VT);
           SDValue FPZero = DAG.getConstantFP(0.0, DL, VT);
           SDValue Fabs = DAG.getNode(ISD::FABS, DL, VT, Op);
           SDValue IsDenorm = DAG.getSetCC(DL, CCVT, Fabs, NormC, ISD::SETLT);
           Est = DAG.getNode(SelOpcode, DL, VT, IsDenorm, FPZero, Est);

llvm/unittests/ADT/CMakeLists.txt

add_llvm_unittest(ADTTests
   DAGDeltaAlgorithmTest.cpp
   DeltaAlgorithmTest.cpp
   DenseMapTest.cpp
   DenseSetTest.cpp
   DepthFirstIteratorTest.cpp
   DirectedGraphTest.cpp
   EquivalenceClassesTest.cpp
   FallibleIteratorTest.cpp
+  FloatingPointMode.cpp
   FoldingSet.cpp
   FunctionExtrasTest.cpp
   FunctionRefTest.cpp
   HashingTest.cpp
   IListBaseTest.cpp
   IListIteratorTest.cpp
   IListNodeBaseTest.cpp
   IListNodeTest.cpp

llvm/unittests/ADT/FloatingPointMode.cpp

+//===- llvm/unittest/ADT/FloatingPointMode.cpp ----------------------------===//
+//
+// 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 "llvm/ADT/FloatingPointMode.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+TEST(FloatingPointModeTest, ParseDenormalFPAttribute) {
+  EXPECT_EQ(DenormalMode::IEEE, parseDenormalFPAttribute("ieee"));
+  EXPECT_EQ(DenormalMode::IEEE, parseDenormalFPAttribute(""));
+  EXPECT_EQ(DenormalMode::PreserveSign,
+            parseDenormalFPAttribute("preserve-sign"));
+  EXPECT_EQ(DenormalMode::PositiveZero,
+            parseDenormalFPAttribute("positive-zero"));
+  EXPECT_EQ(DenormalMode::Invalid, parseDenormalFPAttribute("foo"));
+}
+
+TEST(FloatingPointModeTest, DenormalAttributeName) {
+  EXPECT_EQ("ieee", denormalModeName(DenormalMode::IEEE));
+  EXPECT_EQ("preserve-sign", denormalModeName(DenormalMode::PreserveSign));
+  EXPECT_EQ("positive-zero", denormalModeName(DenormalMode::PositiveZero));
+  EXPECT_EQ("", denormalModeName(DenormalMode::Invalid));
+}
+
+}