[PATCH] D26467: [ThinLTO] Only promote exported locals as marked in index

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

(Hit send too early on previous response)



================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:80
+    // it must be promoted, so unconditionally promote all values in the
+    // importing module.
+    return true;
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > why can't we just promote what is needed?
> > See the comment just above this - when we are importing we may not have a summary in the distributed backend case (we only get the summaries for things we want to import as defs, not all references in the module). At the worst, though, we will simply promote some values that are never referenced after import, so it won't affect the resulting module. 
> > See the comment just above this - when we are importing we may not have a summary in the distributed backend case (we only get the summaries for things we want to import as defs, not all references in the module).
> 
> That's annoying.
> 
> > At the worst, though, we will simply promote some values that are never referenced after import, so it won't affect the resulting module.
> 
> Sure, but I'm still not fan: it means we can't catch error at this stage.
>> At the worst, though, we will simply promote some values that are never referenced after import, so it won't affect the >> resulting module.
> Sure, but I'm still not fan: it means we can't catch error at this stage.

Agree, it would be better to be able to handle these based on the index. However, this means including summaries of references (not to be imported as defs) assuming that the backend's invocation of llvm::ComputeCrossModuleImportForModule will always make the same decisions. If someone for example accidentally passes tuning options like -import-instr-limit to the backend and not the thin link this could break things if a superset of things originally decided for import by the thin link are then imported. A more robust solution would be to add another flag to the (combined) index that identifies summaries to import as definitions.


Repository:
  rL LLVM

https://reviews.llvm.org/D26467