Fix emission of _fltused for MSVC.

[jyknight]

Summary

It should be emitted when any floating-point operations (including
calls) are present in the object, not just when calls to printf/scanf
with floating point args are made.

The difference caused by this is very subtle: in static (/MT) builds,
on x86-32, in a program that uses floating point but doesn't print it,
the default x87 rounding mode may not be set properly upon
initialization.

This commit also removes the walk of the types pointed to by pointer
arguments in calls. (To assist in opaque pointer types migration --
eventually the pointee type won't be available.)

That latter implies that it will no longer consider a call like
scanf("%f", &floatvar) as sufficient to emit _fltused on its
own. And without _fltused, scanf("%f") will abort with error R6002. This
new behavior is unlikely to bite anyone in practice (you'd have to
read a float, and do nothing with it!), and also, is consistent with
MSVC.

[rnk]

I have no idea how you ended up here, but looks good, just a few NFC comments. :)

llvm/include/llvm/IR/Type.h

470 ↗

(On Diff #181061)

Seems unrelated?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

394 ↗

(On Diff #181061)

for (const Instruction &I : instructions(F)) ? That range-for helper is defined near inst_begin.

[jyknight]

llvm/include/llvm/IR/Type.h

470 ↗

(On Diff #181061)

This GraphTraits code is actually how I ended up looking at _fltused in the first place -- it's the only caller, and I was looking at removing uses of subtype_begin/end. :)

It can go in a followup commit, however.

[thakis]

On https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8923423461858190016/+/steps/package_clang/0/stdout I'm getting:

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90
FAIL: lld :: COFF/lto-new-symbol.ll (43503 of 45323)
******************** TEST 'lld :: COFF/lto-new-symbol.ll' FAILED ********************
Script:
--
: 'RUN: at line 2';   /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/bin/llvm-as -o /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-new-symbol.ll.tmp.obj /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/tools/lld/test/COFF/lto-new-symbol.ll
: 'RUN: at line 3';   /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/bin/lld-link /out:/b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-new-symbol.ll.tmp.exe /entry:foo /subsystem:console /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-new-symbol.ll.tmp.obj
--
Exit Code: 1

Command Output (stderr):
--
lld-link: error: undefined symbol: _fltused
>>> referenced by lto.tmp

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.
FAIL: lld :: COFF/lto-lazy-reference.ll (43516 of 45323)
******************** TEST 'lld :: COFF/lto-lazy-reference.ll' FAILED ********************
Script:
--
: 'RUN: at line 2';   /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/bin/llc -mtriple=i686-pc-windows-msvc -filetype=obj -o /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference-quadruple.obj /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/tools/lld/test/COFF/Inputs/lto-lazy-reference-quadruple.ll
: 'RUN: at line 3';   /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/bin/llvm-as -o /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference-dummy.bc /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/tools/lld/test/COFF/Inputs/lto-lazy-reference-dummy.ll
: 'RUN: at line 4';   rm -f /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib
: 'RUN: at line 5';   llvm-ar cru /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference-quadruple.obj /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference-dummy.bc
: 'RUN: at line 6';   /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/bin/llvm-as -o /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.obj /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/tools/lld/test/COFF/lto-lazy-reference.ll
: 'RUN: at line 7';   /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/bin/lld-link /out:/b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.exe /entry:main /subsystem:console /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.obj /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm-bootstrap/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib
--
Exit Code: 1

Command Output (stderr):
--
lld-link: error: undefined symbol: __fltused
>>> referenced by lto.tmp

Could that be due to this change?

[jyknight]

The _fltused symbol being required in those two compilations is expected behavior. What I'm not sure about is why those two tests don't see any definition of the _fltused symbol? I guess they don't link against the C runtime?

[jyknight]

In D56548#1369995, @jyknight wrote:

The _fltused symbol being required in those two compilations is expected behavior. What I'm not sure about is why those two tests don't see any definition of the _fltused symbol? I guess they don't link against the C runtime?

Oh, of course they don't, because they're intended to work on all platforms. I've fixed the tests in r352110 by just defining the symbol. I believe that's the correct fix.

[thakis]

Thanks!

[stella.stamenova]

One of the LLDB bots is still failing because of this change: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/978/steps/test/logs/stdio

[jyknight]

In D56548#1370323, @stella.stamenova wrote:

One of the LLDB bots is still failing because of this change: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/978/steps/test/logs/stdio

Committed r352159 to fix this.

[stella.stamenova]

In D56548#1370711, @jyknight wrote:

In D56548#1370323, @stella.stamenova wrote:

One of the LLDB bots is still failing because of this change: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/978/steps/test/logs/stdio

Committed r352159 to fix this.

Thanks! The Buildbot is back to green.

llvm/trunk/include/llvm/CodeGen/MachineModuleInfo.h

class MachineModuleInfo : public ImmutablePass {
   // synchronous (precise at call-sites only) CFA into .eh_frame. However,
   // even under this switch, we'd like .debug_frame to be precise when using
   // -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 debugging information is available in this module.
   bool DbgInfoAvailable;
 
-  /// True if this module calls VarArg function with floating-point arguments.
-  /// This is used to emit an undefined reference to _fltused on Windows
-  /// targets.
-  bool UsesVAFloatArgument;
+  /// 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;
 
   /// True if the module calls the __morestack function indirectly, as is
   /// required under the large code model on x86. This is used to emit
   /// a definition of a symbol, __morestack_addr, containing the address. See
   /// comments in lib/Target/X86/X86FrameLowering.cpp for more details.
   bool UsesMorestackAddr;
 
   /// True if the module contains split-stack functions. This is used to
public:
   const Ty &getObjFileInfo() const {
     return const_cast<MachineModuleInfo*>(this)->getObjFileInfo<Ty>();
   }
 
   /// Returns true if valid debug info is present.
   bool hasDebugInfo() const { return DbgInfoAvailable; }
   void setDebugInfoAvailability(bool avail) { DbgInfoAvailable = avail; }
 
-  bool usesVAFloatArgument() const {
-    return UsesVAFloatArgument;
-  }
+  bool usesMSVCFloatingPoint() const { return UsesMSVCFloatingPoint; }
 
-  void setUsesVAFloatArgument(bool b) {
-    UsesVAFloatArgument = b;
-  }
+  void setUsesMSVCFloatingPoint(bool b) { UsesMSVCFloatingPoint = b; }
 
   bool usesMorestackAddr() const {
     return UsesMorestackAddr;
   }
 
   void setUsesMorestackAddr(bool b) {
     UsesMorestackAddr = b;
   }
public:
 
   /// Return array of personality functions ever seen.
   const std::vector<const Function *>& getPersonalities() const {
     return Personalities;
   }
   /// \}
 }; // End class MachineModuleInfo
 
-//===- MMI building helpers -----------------------------------------------===//
-
-/// Determine if any floating-point values are being passed to this variadic
-/// function, and set the MachineModuleInfo's usesVAFloatArgument flag if so.
-/// This flag is used to emit an undefined reference to _fltused on Windows,
-/// which will link in MSVCRT's floating-point support.
-void computeUsesVAFloatArgument(const CallInst &I, MachineModuleInfo &MMI);
-
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_MACHINEMODULEINFO_H

llvm/trunk/lib/CodeGen/MachineModuleInfo.cpp

MachineModuleInfo::MachineModuleInfo(const LLVMTargetMachine *TM)
   initializeMachineModuleInfoPass(*PassRegistry::getPassRegistry());
 }
 
 MachineModuleInfo::~MachineModuleInfo() = default;
 
 bool MachineModuleInfo::doInitialization(Module &M) {
   ObjFileMMI = nullptr;
   CurCallSite = 0;
-  UsesVAFloatArgument = UsesMorestackAddr = false;
+  UsesMSVCFloatingPoint = UsesMorestackAddr = false;
   HasSplitStack = HasNosplitStack = false;
   AddrLabelSymbols = nullptr;
   TheModule = &M;
   DbgInfoAvailable = !empty(M.debug_compile_units());
   return false;
 }
 
 bool MachineModuleInfo::doFinalization(Module &M) {

 
 } // end anonymous namespace
 
 char FreeMachineFunction::ID;
 
 FunctionPass *llvm::createFreeMachineFunctionPass() {
   return new FreeMachineFunction();
 }
-
-//===- MMI building helpers -----------------------------------------------===//
-
-void llvm::computeUsesVAFloatArgument(const CallInst &I,
-                                      MachineModuleInfo &MMI) {
-  FunctionType *FT =
-      cast<FunctionType>(I.getCalledValue()->getType()->getContainedType(0));
-  if (FT->isVarArg() && !MMI.usesVAFloatArgument()) {
-    for (unsigned i = 0, e = I.getNumArgOperands(); i != e; ++i) {
-      Type *T = I.getArgOperand(i)->getType();
-      for (auto i : post_order(T)) {
-        if (i->isFloatingPointTy()) {
-          MMI.setUsesVAFloatArgument(true);
-          return;
-        }
-      }
-    }
-  }
-}

llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp

if (const InlineAsm *IA = dyn_cast<InlineAsm>(Call->getCalledValue())) {
 
     BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
             TII.get(TargetOpcode::INLINEASM))
         .addExternalSymbol(IA->getAsmString().c_str())
         .addImm(ExtraInfo);
     return true;
   }
 
-  MachineModuleInfo &MMI = FuncInfo.MF->getMMI();
-  computeUsesVAFloatArgument(*Call, MMI);
-
   // Handle intrinsic function calls.
   if (const auto *II = dyn_cast<IntrinsicInst>(Call))
     return selectIntrinsicCall(II);
 
   // Usually, it does not make sense to initialize a value,
   // make an unrelated function call and use the value, because
   // it tends to be spilled on the stack. So, we move the pointer
   // to the last local value to the beginning of the block, so that

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 
 void SelectionDAGBuilder::visitCall(const CallInst &I) {
   // Handle inline assembly differently.
   if (isa<InlineAsm>(I.getCalledValue())) {
     visitInlineAsm(&I);
     return;
   }
 
-  MachineModuleInfo &MMI = DAG.getMachineFunction().getMMI();
-  computeUsesVAFloatArgument(I, MMI);
-
   const char *RenameFn = nullptr;
   if (Function *F = I.getCalledFunction()) {
     if (F->isDeclaration()) {
       // Is this an LLVM intrinsic or a target-specific intrinsic?
       unsigned IID = F->getIntrinsicID();
       if (!IID)
         if (const TargetIntrinsicInfo *II = TM.getIntrinsicInfo())
           IID = II->getIntrinsicID(F);

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

 #include "llvm/CodeGen/ISDOpcodes.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineMemOperand.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachinePassRegistry.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/SchedulerRegistry.h"
 #include "llvm/CodeGen/SelectionDAG.h"
 #include "llvm/CodeGen/SelectionDAGNodes.h"
 #include "llvm/CodeGen/StackProtector.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetLowering.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/CodeGen/ValueTypes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/InlineAsm.h"
+#include "llvm/IR/InstIterator.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/User.h"
for (BasicBlock::iterator I = BB.begin(); (PN = dyn_cast<PHINode>(I)); ++I)
         SplitCriticalEdge(
             Pred->getTerminator(), GetSuccessorNumber(Pred, &BB),
             CriticalEdgeSplittingOptions(DT, LI).setMergeIdenticalEdges());
         goto ReprocessBlock;
       }
   }
 }
 
+static void computeUsesMSVCFloatingPoint(const Triple &TT, const Function &F,
+                                         MachineModuleInfo &MMI) {
+  // Only needed for MSVC
+  if (!TT.isKnownWindowsMSVCEnvironment())
+    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;
+      }
+    }
+  }
+}
+
 bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
   // If we already selected that function, we do not need to run SDISel.
   if (mf.getProperties().hasProperty(
           MachineFunctionProperties::Property::Selected))
     return false;
   // Do some sanity-checking on the command-line options.
   assert((!EnableFastISelAbort || TM.Options.EnableFastISel) &&
          "-fast-isel-abort > 0 requires -fast-isel");
for (const auto &MI : MBB) {
         MF->setHasInlineAsm(true);
       }
     }
   }
 
   // Determine if there is a call to setjmp in the machine function.
   MF->setExposesReturnsTwice(Fn.callsFunctionThatReturnsTwice());
 
+  // Determine if floating point is used for msvc
+  computeUsesMSVCFloatingPoint(TM.getTargetTriple(), Fn, MF->getMMI());
+
   // Replace forward-declared registers with the registers containing
   // the desired value.
   MachineRegisterInfo &MRI = MF->getRegInfo();
   for (DenseMap<unsigned, unsigned>::iterator
        I = FuncInfo->RegFixups.begin(), E = FuncInfo->RegFixups.end();
        I != E; ++I) {
     unsigned From = I->first;
     unsigned To = I->second;

llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp

if (TT.isOSBinFormatMachO()) {
     FM.serializeToFaultMapSection();
 
     // This flag tells the linker that no global symbols contain code that fall
     // through to other global symbols (e.g. an implementation of multiple entry
     // points). If this doesn't occur, the linker can safely perform dead code
     // stripping. Since LLVM never generates code that does this, it is always
     // safe to set.
     OutStreamer->EmitAssemblerFlag(MCAF_SubsectionsViaSymbols);
-    return;
-  }
-
-  if (TT.isKnownWindowsMSVCEnvironment() && MMI->usesVAFloatArgument()) {
+  } else if (TT.isOSBinFormatCOFF()) {
+    if (MMI->usesMSVCFloatingPoint()) {
+      // 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:
+      //
+      // 1. For x86-32, it will set the x87 rounding mode to 53-bit instead of
+      // 64-bit mantissas at program start.
+      //
+      // 2. It links in support routines for floating-point in scanf and printf.
+      //
+      // MSVC emits an undefined reference to _fltused when there are any
+      // floating point operations in the program (including calls). A program
+      // that only has: `scanf("%f", &global_float);` may fail to trigger this,
+      // but oh well...that's a documented issue.
       StringRef SymbolName =
-        (TT.getArch() == Triple::x86_64) ? "_fltused" : "__fltused";
+          (TT.getArch() == Triple::x86) ? "__fltused" : "_fltused";
       MCSymbol *S = MMI->getContext().getOrCreateSymbol(SymbolName);
       OutStreamer->EmitSymbolAttribute(S, MCSA_Global);
       return;
     }
-
-  if (TT.isOSBinFormatCOFF()) {
     emitStackMaps(SM);
-    return;
-  }
-
-  if (TT.isOSBinFormatELF()) {
+  } else if (TT.isOSBinFormatELF()) {
     emitStackMaps(SM);
     FM.serializeToFaultMapSection();
-    return;
   }
 }
 
 //===----------------------------------------------------------------------===//
 // Target Registry Stuff
 //===----------------------------------------------------------------------===//
 
 // Force static initialization.
 extern "C" void LLVMInitializeX86AsmPrinter() {
   RegisterAsmPrinter<X86AsmPrinter> X(getTheX86_32Target());
   RegisterAsmPrinter<X86AsmPrinter> Y(getTheX86_64Target());
 }

llvm/trunk/test/CodeGen/X86/fltused.ll

-; The purpose of this test to verify that the fltused symbol is emitted when
-; any function is called with floating point arguments on Windows. And that it
-; is not emitted otherwise.
+; The purpose of this test to verify that the fltused symbol is
+; emitted when a floating point call is made on Windows.
 
 ; RUN: llc < %s -mtriple i686-pc-win32 | FileCheck %s --check-prefix WIN32
 ; RUN: llc < %s -mtriple x86_64-pc-win32 | FileCheck %s --check-prefix WIN64
 ; RUN: llc < %s -O0 -mtriple i686-pc-win32 | FileCheck %s --check-prefix WIN32
 ; RUN: llc < %s -O0 -mtriple x86_64-pc-win32 | FileCheck %s --check-prefix WIN64
 
 @.str = private constant [4 x i8] c"%f\0A\00"
 

llvm/trunk/test/CodeGen/X86/fltused_function_pointer.ll

-; The purpose of this test to verify that the fltused symbol is emitted when
-; any function is called with floating point arguments on Windows. And that it
-; is not emitted otherwise.
+; The purpose of this test to verify that the fltused symbol is
+; emitted when a floating point call is made on Windows.
 
 ; RUN: llc < %s -mtriple i686-pc-win32 | FileCheck %s --check-prefix WIN32
 ; RUN: llc < %s -mtriple x86_64-pc-win32 | FileCheck %s --check-prefix WIN64
 ; RUN: llc < %s -O0 -mtriple i686-pc-win32 | FileCheck %s --check-prefix WIN32
 ; RUN: llc < %s -O0 -mtriple x86_64-pc-win32 | FileCheck %s --check-prefix WIN64
 
 @.str = private constant [4 x i8] c"%f\0A\00"
 
 define i32 @foo(i32 (i8*, ...)* %f) nounwind {
 entry:
   %call = tail call i32 (i8*, ...) %f(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), double 1.000000e+000) nounwind
   ret i32 0
 }
 
 ; WIN32: .globl __fltused
 ; WIN64: .globl _fltused

llvm/trunk/test/CodeGen/X86/fltused_math.ll

+; The purpose of this test to verify that the fltused symbol is
+; emitted when floating point operations are used on Windows.
+
+; RUN: llc < %s -mtriple i686-pc-win32 | FileCheck %s --check-prefix WIN32
+; RUN: llc < %s -mtriple x86_64-pc-win32 | FileCheck %s --check-prefix WIN64
+; RUN: llc < %s -O0 -mtriple i686-pc-win32 | FileCheck %s --check-prefix WIN32
+; RUN: llc < %s -O0 -mtriple x86_64-pc-win32 | FileCheck %s --check-prefix WIN64
+
+define i32 @foo(i32 %a) nounwind {
+entry:
+  %da = sitofp i32 %a to double
+  %div = fdiv double %da, 3.100000e+00
+  %res = fptosi double %div to i32
+  ret i32 %res
+}
+
+; WIN32: .globl __fltused
+; WIN64: .globl _fltused