[PATCH] D26063: [ThinLTO] Use NoPromote flag in summary during promotion

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/Transforms/Utils/FunctionImportUtils.h:44
   /// Check if we should promote the given local value to global scope.
   bool doPromoteLocalToGlobal(const GlobalValue *SGV);
 
----------------
mehdi_amini wrote:
> The name should be "shouldPromoteLocalToGlobal", the "do" added some cognitive load for me to read this patch (I'm not saying that it should be done as part of this patch, but either before or after would be a nice tweak).
Sure. I have this one ready to go, so will do the rename in an immediately following patch.


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:70
 
-  if (GVar && GVar->hasSection())
-    // Some sections like "__DATA,__cfstring" are "magic" and promotion is not
-    // allowed. Just disable promotion on any GVar with sections right now.
+  auto *Summary = ImportIndex.getGlobalValueSummary(SGV->getGUID());
+  assert(Summary && "Missing summary for global value");
----------------
mehdi_amini wrote:
> This `SGV->getGUID()` is the cause for the "DoPromote" right?
Right.


https://reviews.llvm.org/D26063