<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 11 August 2014 15:24, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">ping. Rebased patch attached.<br></blockquote><div><br></div>
<div>Okay. So at a high level, you've decided not to use libLTO any more. Funny aside, the first version of the gold plugin I put in tools/silver and it was written to use the llvm C++ API directly. I think it was Chris who told me that this was silly and I should be reusing the logic from libLTO to do it, so I started a second version in tools/gold.</div>
<div><br></div><div>I'm very sympathetic to the idea of not fixing libLTO and letting it rot (oops, I meant to say letting Apple maintain it for ld64), though not doing that would make ultimately make libLTO stronger. I'm calling this out as a deliberate choice we have to make, we can either push libLTO to being well enough designed to handle linkers with very different plugin ideologies (and thus useful for others), or we can consign it to being an ld64 feature. If gold can't use libLTO because its interface is too limiting, it's likely lld won't either, that any third party linkers won't, etc. No I don't have any suggestions for how to generalize libLTO's API.</div>
<div><br></div><div><br></div><div>+//===- Utils.h - LLVM LTO Utilities ---------------------------------------===//<br></div><div><br></div><div>Missing emacs major mode marker.</div><div><br></div><div><div>+void runLTOPasses(Module &M, TargetMachine &TM, bool DisableOpt,<br>
</div><div>+ bool DisableInline, bool DisableGVNLoadPRE);</div><div><br></div></div><div>This should go away and become an interface to PassManagerBuilder instead. (PMB doesn't actually run the passes, but that's simple for the caller.)</div>
<div><br></div><div><div>+ ErrorOr<object::IRObjectFile *> ObjOrErr =</div><div>+ object::IRObjectFile::createIRObjectFile(std::move(buffer), Context);</div><div>+ std::error_code EC = ObjOrErr.getError();</div>
<div>+ if (EC == BitcodeError::InvalidBitcodeSignature)</div></div><div><br></div><div>What about BitcodeError::InvalidBitcodeHeaderWrapper? Come to think of it, do we even have any tools in mainline that wrap bitcode?</div>
<div><br></div><div><div>+ for (auto &Sym : Obj->symbols()) {</div><div>+ uint32_t Symflags = Sym.getFlags();</div><div>+ if (!(Symflags & object::BasicSymbolRef::SF_Global))</div><div>+ continue;</div>
</div><div><br></div><div>Why are we using the libobject interface for something that must be IR? This line:</div><div><br></div><div>+ const GlobalValue *GV = Obj->getSymbolGV(Sym.getRawDataRefImpl());<br></div><div>
<br></div><div>is horrible. Is this really what it looks like? Type unsafe laundering through void pointers or the equivalent? "Oh sure buddy, there's a GlobalValue* there, I promise."</div><div><br></div><div>
<div>+ assert(!GV->hasExternalWeakLinkage() &&</div><div>+ !GV->hasAvailableExternallyLinkage());</div></div><div><br></div><div>Assert message? Also, hold on, couldn't this happen if we have extern_weak or available_externally in the .bc file? Or is this safe due to behind the scenes IRObjectFile magic?</div>
<div><br></div><div><div>+ if (GV && (GV->hasWeakLinkage() || GV->hasLinkOnceLinkage()))</div><div>+ sym.comdat_key = (char*)1;</div></div><div><br></div><div>Let me get this straight. The old code called strdup(M->getSymbolName) to fill <a href="http://sym.name">sym.name</a>, then upon finding comdat did sym.comday_key = <a href="http://sym.name">sym.name</a>. The new code adds a "std::string NameBuffer" per claimed file, then we raw_ostream the names into that NameBuffer as we visit each symbol and you mark the ones that need a comdat_key here, then after this you have a loop that strlen's over NameBuffer to fill in <a href="http://sym.name">sym.name</a> (and sym.comdat_key if was set to 1 here).</div>
<div><br></div><div>Why? Firstly this seems like a separable change. Secondly, did this use to leak, or did you introduce a double-free when gold tries to free it and then we delete NameBuffer? Third, would a BumpPtrAllocator<char> be better here? You get slabs that don't individually move, so you can set the pointer once instead of doing it in a loop after the fact.</div>
<div><br></div><div>+static void keepGlobalValue(GlobalValue &GV) {<br></div><div><br></div><div>I've been pontificating over what it means to have llvm.used name a function with a discardable linkage. The idea is that it would be one way to communicate to internalize that it should form weak linkage out of it. I'm not sure whether that's safe, it means that attribute used would run the risk of changing linkages.</div>
<div><br></div><div>+ std::vector<GlobalValue*> Drop;<br></div><div><br></div><div>Space before *.</div><div><br></div><div><div>+ // Since we use the regular lib/Linker, we cannot just internalized</div><div>
+ // it now or it will not be copied to the merged module. Instead</div><div>+ // we force it to be copied and then internalize it.</div></div><div><br></div><div>Typo or grammar error starting at "internalized".</div>
<div><br></div><div>+ // We can only check for address uses once we merge the modules. The<br></div><div><br></div><div>s/once/after/ . As written it causes a "garden path" parse.</div><div><br></div><div>
+ // This is horrible. Given how lazy loading is implemneted, dropping<br>
</div><div><br></div><div>Typo, "implemneted" -> "implemented". Also, that really is a serious design flaw. Oof.</div><div><div><br></div></div><div>+ message(LDPL_FATAL, "Failed to link moudle: %s", ErrMsg.c_str());<br>
</div><div><br></div><div>Typo, "moudle" -> "module".</div><div><br></div><div><div>+ if (canBeOmittedFromSymbolTable(GV))</div><div>+ internalize(*GV);</div></div><div><br></div><div>Three space indent? Please clang-format.</div>
<div><br></div><div>Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5">On 6 August 2014 11:32, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
>> I don't think we drop them yet, but I see it is as just a convenience<br>
>> so that it is easy to test it with llc. While developing this patch I<br>
>> did a few clang bootstraps and only once noticed a unused linkonce_odr<br>
>> in a .bc produced by clang. The previous passes should drop it. We<br>
>> could add an option to do it, I am just not sure it would have a big<br>
>> impact (I will write a patch to check).<br>
><br>
> And I can't even reproduce that anymore.<br>
><br>
> I applied the attached patch to llvm to make it assert when writing<br>
> bitcode or assembly with a dead linkonce_odr. It will correctly crash<br>
> in llvm-as or llc when given just<br>
><br>
> define linkonce_odr void @g() {<br>
> ret void<br>
> }<br>
><br>
> But it can successfully build a stage2 with or without LTO, so it<br>
> looks we are pretty good at dropping unused linkonce_odr and dropping<br>
> it in codegen proper would not add any real value.<br>
><br>
> Cheers,<br>
> Rafael<br>
</div></div></blockquote></div><br></div></div>