<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jul 27, 2017 at 7:12 AM Teresa Johnson via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">tejohnson added a comment.<br>
<br>
Sorry for the delay. A few comments below. Regarding Mehdi's performance question - I don't expect this to have a major impact as we were only importing aliases in limited cases anyway. We'll see any performance effect in our nightly testing once this goes in, which should surface any surprise effects.<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/IPO/FunctionImport.cpp:135<br>
return false;<br>
if (auto *AS = dyn_cast<AliasSummary>(GVSummary)) {<br>
+ // Aliases can't point to "available_externally".<br>
----------------<br>
Braces can be removed<br></blockquote><div><br>I go back and forth on this a bit - some people only omit {} when the body is a single line (not just a single statement) - given the 3 line comment it feels a bit better to me to include the braces (even though I'll omit them on something like "for / if / for / return" nested 4 levels deep when the first for then does have a 3 line body without braces - for some reason the extra levels of indentation and their proximity are easier to read than a long similarly indented body without a brace at the end... beats me)<br><br>Removed them here, though (:<br><br>Applied the rest of the fixes as well.<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: lib/Transforms/IPO/FunctionImport.cpp:138<br>
// FIXME: we should import alias as available_externally *function*,<br>
// the destination module does need to know it is an alias.<br>
+ return false;<br>
----------------<br>
While you're in here, can you correct the comment: s/does needs to know/does not need to know/<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/IPO/FunctionImport.cpp:228<br>
const FunctionSummary *ResolvedCalleeSummary;<br>
if (isa<AliasSummary>(CalleeSummary)) {<br>
ResolvedCalleeSummary = cast<FunctionSummary>(<br>
----------------<br>
This handling needs to be updated - can simply assert that CalleeSummary not an AliasSummary.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:27<br>
// For alias, we tie the definition to the base object. Extract it and recurse<br>
- if (auto *GA = dyn_cast<GlobalAlias>(SGV)) {<br>
- if (GA->isInterposable())<br>
- return false;<br>
- const GlobalObject *GO = GA->getBaseObject();<br>
- if (!GO->hasLinkOnceODRLinkage())<br>
- return false;<br>
- return FunctionImportGlobalProcessing::doImportAsDefinition(<br>
- GO, GlobalsToImport);<br>
- }<br>
+ if (auto *GA = dyn_cast<GlobalAlias>(SGV))<br>
+ return false;<br>
----------------<br>
GA is unused, and comment is stale. But I also think that this should be moved to after the below check of SGV being in GlobalsToImport, and converted to an assert (if in GlobalsToImport, better not be an alias).<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:133<br>
case GlobalValue::ExternalLinkage:<br>
// External defnitions are converted to available_externally<br>
// definitions upon import, so that they are available for inlining<br>
----------------<br>
"External and linkonce definitions" (can you fix typo in "defnitions" while here)?<br>
<br>
<br>
================<br>
Comment at: test/ThinLTO/X86/alias_import.ll:48<br>
<br>
; On the import side now, verify that aliases to a linkonce_odr are imported, but the weak/linkonce (we can't inline them)<br>
; IMPORT-DAG: declare void @linkonceODRfuncWeakAlias<br>
----------------<br>
Comment needs update.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D35875" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35875</a><br>
<br>
<br>
<br>
</blockquote></div></div>