[PATCH] D55627: [ThinLTO] Remove dllimport attribute from locally defined symbols

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: include/llvm/IR/GlobalValue.h:198-199
+      setDSOLocal(true);
+      if (hasDLLImportStorageClass())
+        setDLLStorageClass(DLLStorageClassTypes::DefaultStorageClass);
+    }
----------------
This change doesn't seem quite right. On the one hand, if a GlobalValue has internal linkage, it probably shouldn't be marked dllimport. However, we really shouldn't look at visibility or external weak linkage, which are concepts that don't exist for COFF. And we probably shouldn't adjust the dll storage class from a function named `maybeSetDsoLocal`.


================
Comment at: lib/LTO/LTO.cpp:698-700
+        if (GV->hasDLLImportStorageClass())
+          GV->setDLLStorageClass(GlobalValue::DLLStorageClassTypes::
+                                 DefaultStorageClass);
----------------
This looks like the important functional change. Would the test still pass if you reverted the changes other than this one?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55627/new/

https://reviews.llvm.org/D55627