[TargetMachine] Move COFF special case for ExternalSymbolSDNode from shouldAssumeDSOLocal to X86Subtarget

[MaskRay]

Summary

Intended to be NFC. ARM/AArch64 don't appear to need adjustment.

TargetMachine::shouldAssumeDSOLocal is expected to be very simple, ideally
matching isDSOLocal(). The IR producers are expected to set dso_local correctly.
(While some may think this function can make producers' work easier, the
function is really not in a good position to set dso_local. See the various
special cases we duplicate from clang CodeGenModule.cpp.)

[mstorsjo]

Intended to be NFC. ARM/AArch64 don't appear to need adjustment.

Hmm, it seems like the COFF specific code that this refers to has gone through a couple refactorings since I touched it, so I don't remember exactly which things it refers to and when that check was needed. My main concern is if the conclusion that the ARM/AArch64 codepaths don't need adjustment comes from some supposedly COFF-generic test only is executed for x86.

It's also plausible that it just isn't needed there, because those corresponding arm/aarch64 subtarget functions for classifying references are indeed a little bit different than the x86 one.

In practice, I did include this patch in my nightly test run (running things for all 4 architectures, which should try out many of the mingw specific edge cases) and didn't run into any issues with it - so I guess it should be practically fine...

[MaskRay]

FYI I use this script to test end-to-end behavior of .c => .s: https://gist.github.com/MaskRay/c03a90922003df666551589f1629df22

The following configurations's codegen stay unchanged. So this should be safe.

run win.x86 -target x86_64-windows
run win.x86.asan -target x86_64-windows -fsanitize=address
run win.x86.coverage -target x86_64-windows -fprofile-instr-generate
run win.x86.pgo -target x86_64-windows -fprofile-generate

run mingw.x86 -target x86_64-windows-gnu
run mingw.x86.nopic -target x86_64-windows-gnu -fno-pic
run mingw.x86.asan -target x86_64-windows-gnu -fsanitize=address
run mingw.x86.coverage -target x86_64-windows-gnu -fprofile-instr-generate
run mingw.x86.pgo -target x86_64-windows-gnu -fprofile-generate

run win.arm -target arm-windows
run win.arm.asan -target arm-windows -fsanitize=address

run win.a64 -target aarch64-windows
run win.a64.asan -target aarch64-windows -fsanitize=address

If there is arm/aarch64 coverage issue, it can't be this patch's fault:)

[mstorsjo]

In D108514#2960293, @MaskRay wrote:

FYI I use this script to test end-to-end behavior of .c => .s: https://gist.github.com/MaskRay/c03a90922003df666551589f1629df22

Awesome, thanks for being thorough in checking it!

I can't come up with anything else right now to check, so I guess there's no reason to hold this back then.

llvm/lib/Target/TargetMachine.cpp

bool TargetMachine::shouldAssumeDSOLocal(const Module &M,
   const Triple &TT = getTargetTriple();
   Reloc::Model RM = getRelocationModel();
 
   // According to the llvm language reference, we should be able to
   // just return false in here if we have a GV, as we know it is
   // dso_preemptable.  At this point in time, the various IR producers
   // have not been transitioned to always produce a dso_local when it
   // is possible to do so.
-  // In the case of ExternalSymbolSDNode, GV is null and we should just return
-  // false. However, COFF currently relies on this to be true
   //
   // As a result we still have some logic in here to improve the quality of the
   // generated code.
-  // FIXME: Add a module level metadata for whether intrinsics should be assumed
-  // local.
   if (!GV)
-    return TT.isOSBinFormatCOFF();
+    return false;
 
   // If the IR producer requested that this GV be treated as dso local, obey.
   if (GV->isDSOLocal())
     return true;
 
   if (TT.isOSBinFormatCOFF()) {
     // DLLImport explicitly marks the GV as external.
     if (GV->hasDLLImportStorageClass())

llvm/lib/Target/X86/X86Subtarget.cpp

if (Optional<ConstantRange> CR = GV->getAbsoluteSymbolRange()) {
         return X86II::MO_NO_FLAG;
     }
   }
 
   if (TM.shouldAssumeDSOLocal(M, GV))
     return classifyLocalReference(GV);
 
   if (isTargetCOFF()) {
+    // ExternalSymbolSDNode like _tls_index.
+    if (!GV)
+      return X86II::MO_NO_FLAG;
     if (GV->hasDLLImportStorageClass())
       return X86II::MO_DLLIMPORT;
     return X86II::MO_COFFSTUB;
   }
   // Some JIT users use *-win32-elf triples; these shouldn't use GOT tables.
   if (isOSWindows())
     return X86II::MO_NO_FLAG;
 

 }
 
 unsigned char
 X86Subtarget::classifyGlobalFunctionReference(const GlobalValue *GV,
                                               const Module &M) const {
   if (TM.shouldAssumeDSOLocal(M, GV))
     return X86II::MO_NO_FLAG;
 
-  // Functions on COFF can be non-DSO local for two reasons:
+  // Functions on COFF can be non-DSO local for three reasons:
+  // - They are intrinsic functions (!GV)
   // - They are marked dllimport
   // - They are extern_weak, and a stub is needed
   if (isTargetCOFF()) {
+    if (!GV)
+      return X86II::MO_NO_FLAG;
     if (GV->hasDLLImportStorageClass())
       return X86II::MO_DLLIMPORT;
     return X86II::MO_COFFSTUB;
   }
 
   const Function *F = dyn_cast_or_null<Function>(GV);
 
   if (isTargetELF()) {