CodeGen: Remove UsesMSVCFloatingPoint from MachineModuleInfo

[arsenm]

This is only used by x86 and only used in the AsmPrinter module pass. I
think implementing this by looking at the underlying IR types instead
of the selected instructions is a pretty horrifying implementation,
but it's still available in the AsmPrinter.

This is https://reviews.llvm.org/D123933 resurrected.

I still don't know what the point of emitting _fltused is, but this
approach of looking at the IR types probably isn't the right way to
do this in the first place. If the intent is report any FP instructions,
this will miss any implicitly introduced ones during codegen. Also don't
know why just unconditionally emitting it isn't an option.

The last review mentioned the ARMs might want to emit this, but I'm
not going to go fix that. If someone wants to emit this on ARM, they
can move this to a common helper or analysis somewhere.

[arsenm]

• CodeGen: Remove MachineModuleInfo reference from MachineFunction #100357
• DebugInfo: Avoid some MMI::hasDebugInfo checks #100333
• CodeGen: Move current call site out of MachineModuleInfo #100369
• CodeGen: Remove UsesMSVCFloatingPoint from MachineModuleInfo #100368 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @arsenm and the rest of your teammates on Graphite

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-platform-windows

Author: Matt Arsenault (arsenm)

Changes

This is only used by x86 and only used in the AsmPrinter module pass. I
think implementing this by looking at the underlying IR types instead
of the selected instructions is a pretty horrifying implementation,
but it's still available in the AsmPrinter.

This is https://reviews.llvm.org/D123933 resurrected.

I still don't know what the point of emitting _fltused is, but this
approach of looking at the IR types probably isn't the right way to
do this in the first place. If the intent is report any FP instructions,
this will miss any implicitly introduced ones during codegen. Also don't
know why just unconditionally emitting it isn't an option.

The last review mentioned the ARMs might want to emit this, but I'm
not going to go fix that. If someone wants to emit this on ARM, they
can move this to a common helper or analysis somewhere.

---

Full diff: https://github.com/llvm/llvm-project/pull/100368.diff

4 Files Affected:

• (modified) llvm/include/llvm/CodeGen/MachineModuleInfo.h (-8)
• (modified) llvm/lib/CodeGen/MachineModuleInfo.cpp (-1)
• (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (-27)
• (modified) llvm/lib/Target/X86/X86AsmPrinter.cpp (+24-1)

diff --git a/llvm/include/llvm/CodeGen/MachineModuleInfo.h b/llvm/include/llvm/CodeGen/MachineModuleInfo.h
index b39db93b021b5..f69be67ee9f17 100644
--- a/llvm/include/llvm/CodeGen/MachineModuleInfo.h
+++ b/llvm/include/llvm/CodeGen/MachineModuleInfo.h
@@ -113,10 +113,6 @@ class MachineModuleInfo {
   // -g. At this moment, there's no way to specify that some CFI directives
   // go into .eh_frame only, while others go into .debug_frame only.
 
-  /// True if this module is being built for windows/msvc, and uses floating
-  /// point.  This is used to emit an undefined reference to _fltused.
-  bool UsesMSVCFloatingPoint = false;
-
   /// Maps IR Functions to their corresponding MachineFunctions.
   DenseMap<const Function*, std::unique_ptr<MachineFunction>> MachineFunctions;
   /// Next unique number available for a MachineFunction.
@@ -183,10 +179,6 @@ class MachineModuleInfo {
     return const_cast<MachineModuleInfo*>(this)->getObjFileInfo<Ty>();
   }
 
-  bool usesMSVCFloatingPoint() const { return UsesMSVCFloatingPoint; }
-
-  void setUsesMSVCFloatingPoint(bool b) { UsesMSVCFloatingPoint = b; }
-
   /// \name Exception Handling
   /// \{
 
diff --git a/llvm/lib/CodeGen/MachineModuleInfo.cpp b/llvm/lib/CodeGen/MachineModuleInfo.cpp
index 12dec288b3ce2..23de726a2ab97 100644
--- a/llvm/lib/CodeGen/MachineModuleInfo.cpp
+++ b/llvm/lib/CodeGen/MachineModuleInfo.cpp
@@ -38,7 +38,6 @@ void MachineModuleInfo::initialize() {
   ObjFileMMI = nullptr;
   CurCallSite = 0;
   NextFnNum = 0;
-  UsesMSVCFloatingPoint = false;
 }
 
 void MachineModuleInfo::finalize() {
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index 401d23b22adcd..84331d257a3d0 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -417,30 +417,6 @@ void SelectionDAGISelLegacy::getAnalysisUsage(AnalysisUsage &AU) const {
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
-static void computeUsesMSVCFloatingPoint(const Triple &TT, const Function &F,
-                                         MachineModuleInfo &MMI) {
-  // Only needed for MSVC
-  if (!TT.isWindowsMSVCEnvironment())
-    return;
-
-  // If it's already set, nothing to do.
-  if (MMI.usesMSVCFloatingPoint())
-    return;
-
-  for (const Instruction &I : instructions(F)) {
-    if (I.getType()->isFPOrFPVectorTy()) {
-      MMI.setUsesMSVCFloatingPoint(true);
-      return;
-    }
-    for (const auto &Op : I.operands()) {
-      if (Op->getType()->isFPOrFPVectorTy()) {
-        MMI.setUsesMSVCFloatingPoint(true);
-        return;
-      }
-    }
-  }
-}
-
 PreservedAnalyses
 SelectionDAGISelPass::run(MachineFunction &MF,
                           MachineFunctionAnalysisManager &MFAM) {
@@ -802,9 +778,6 @@ bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
     }
   }
 
-  // Determine if floating point is used for msvc
-  computeUsesMSVCFloatingPoint(TM.getTargetTriple(), Fn, *CurDAG->getMMI());
-
   // Release function-specific state. SDB and CurDAG are already cleared
   // at this point.
   FuncInfo->clear();
diff --git a/llvm/lib/Target/X86/X86AsmPrinter.cpp b/llvm/lib/Target/X86/X86AsmPrinter.cpp
index 0c2c6bf7f8b70..9d86a9c9d1609 100644
--- a/llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ b/llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -28,6 +28,7 @@
 #include "llvm/CodeGenTypes/MachineValueType.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/InlineAsm.h"
+#include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
@@ -975,6 +976,28 @@ static void emitNonLazyStubs(MachineModuleInfo *MMI, MCStreamer &OutStreamer) {
   }
 }
 
+/// True if this module is being built for windows/msvc, and uses floating
+/// point.  This is used to emit an undefined reference to _fltused.
+static bool usesMSVCFloatingPoint(const Triple &TT, const Module &M) {
+  // Only needed for MSVC
+  if (!TT.isWindowsMSVCEnvironment())
+    return false;
+
+  for (const Function &F : M) {
+    for (const Instruction &I : instructions(F)) {
+      if (I.getType()->isFPOrFPVectorTy())
+        return true;
+
+      for (const auto &Op : I.operands()) {
+        if (Op->getType()->isFPOrFPVectorTy())
+          return true;
+      }
+    }
+  }
+
+  return false;
+}
+
 void X86AsmPrinter::emitEndOfAsmFile(Module &M) {
   const Triple &TT = TM.getTargetTriple();
 
@@ -993,7 +1016,7 @@ void X86AsmPrinter::emitEndOfAsmFile(Module &M) {
     // safe to set.
     OutStreamer->emitAssemblerFlag(MCAF_SubsectionsViaSymbols);
   } else if (TT.isOSBinFormatCOFF()) {
-    if (MMI->usesMSVCFloatingPoint()) {
+    if (usesMSVCFloatingPoint(TT, M)) {
       // In Windows' libcmt.lib, there is a file which is linked in only if the
       // symbol _fltused is referenced. Linking this in causes some
       // side-effects:

[RKSimon]

LGTM - cheers

[compnerd]

_fltused is used to identify the need for supporting functions and is used during the linking process. Always emitting results in code size increases and overlinking, but catching the generated ones is definitely something that we should be doing.

[arsenm]

_fltused is used to identify the need for supporting functions and is used during the linking process. Always emitting results in code size increases and overlinking, but catching the generated ones is definitely something that we should be doing.

So really it should be looking for post-legalize calls to specific runtime libcalls

[compnerd]

Not entirely - _fltused indicates that any floating point operations are being used. The problem is that the kernel mode code does not permit floating point operations (though you can explicitly use KeSaveFloatingPointState and KeRestoreFloatingPointState to explicitly use floating point code in a region). The _fltused is a protection mechanism for that, with the user space C++ runtime ("ABI library" - a la libc++abi) support library (vcruntime) providing a default definition of the symbol. That way, if the user accidentally uses floating point code in the kernel, it is identified at link time and prevents the errant use. Doing this post legalization makes sense though.

[rnk]

I think @jyknight touched this last, and I may have advised him to simply use MMI to store this, since historically it has been used as a side channel for passing information along the code generation pipeline. It's good to avoid that, but it was what we were doing at the time. :)

I think the reason the existing scan was done as part of SDISel is that it is a scan over the IR, and ISel is the point at which we walk the IR to build the mostly-standalone MIR representation. Moving this scan to AsmPrinting is a bit distasteful, but it works and is less code. You could also make a cache-locality argument that walking the IR again during AsmPrinting is slow, but that's pretty weak.

Scanning the IR for FP types is obviously a gross approximation of _fltused. If we want to try to implement this properly, maybe we could scan all MachineInstrs during AsmPrinting for usage of vector or x87 FP operands. We can leave that out of scope, though.

In conclusion, we should land this as is, but I wanted to share some context.

[jyknight]

It's been a while since I looked at this, but its purpose, at least back then (2c36240) is very different than what was discussed above.

It had two effects:

1. decide whether to include floating-point support in printf/scanf library routines.
2. decide whether to include code to modify the x87 FPU mode flags at program startup to the expected values.

[jyknight]

In particular, I think it would be incorrect to just look at floating-point operations at the MIR level, because calling printf with a constant floating point argument doesn't necessarily use any floating point instructions.

[arsenm]

Merge activity

• Jul 26, 3:21 AM EDT: @arsenm started a stack merge that includes this pull request via Graphite.
• Jul 26, 3:24 AM EDT: Graphite rebased this pull request as part of a merge.
• Jul 26, 3:27 AM EDT: @arsenm merged this pull request with Graphite.