<div dir="ltr">No problem. Thanks for committing it! I'm not sure how to create test cases and C bindings yet, but I will figure it out submit for review.</div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jul 29, 2016 at 9:10 PM Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Sean,</div><div dir="ltr"><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"><span style="font-size:12.800000190734863px">As for the part that couples them too tightly, would you recommend I just keep my own specialized version of CompileOnDemandLayer.h that includes this functionality, or do you have any ideas for a cleaner way to do this?</span></blockquote><div><span style="font-size:12.800000190734863px"><br></span></div></div><div dir="ltr"><div><span style="font-size:12.800000190734863px">My apologies - I wasn't very clear in my description of the issue. The only sense in which your original patch was tightly coupled was that it had getLogicalModuleResourcesForSymbol accessing LogicalModuleResources::SourceModule, which isn't a required part of the 'LogicalModuleResources' interface at the moment. The alternative implementation of getLogicalModuleResourcesForSymbol doesn't have that coupling, and since it works for you I think the issue is resolved. I've </span>committed<span style="font-size:12.800000190734863px"> your patch with my suggested modification in r</span>277257.</div><div><span style="font-size:12.800000190734863px"><br></span></div><div><span style="font-size:12.800000190734863px">Thanks very much for contributing this - recompilation support is a </span>popular<span style="font-size:12.800000190734863px"> request as you noted,</span><span style="font-size:12.800000190734863px"> and it'd be great to have a good interface for it.</span></div><div><span style="font-size:12.800000190734863px"><br></span></div><div><span style="font-size:12.800000190734863px">There is still a lot more that can be done to support re-optimization: C bindings, test cases, on-stack counts for </span>functions<span style="font-size:12.800000190734863px"> that might be replaced (so that we can </span>release<span style="font-size:12.800000190734863px"> their memory once they're no longer running), and more. If you continue working on this please keep submitting patches and I'll be happy to review them. The standard </span>process is to create a review on<span style="font-size:12.800000190734863px"> <a href="http://reviews.llvm.org" target="_blank">reviews.llvm.org</a> (set me as the reviewer if it's JIT related), or post a diff to the llvm-commits mailing list (and CC me).</span></div><div><span style="font-size:12.800000190734863px"><br></span></div><div><span style="font-size:12.800000190734863px">Thanks again Sean!</span></div><div><span style="font-size:12.800000190734863px"><br></span></div><div><span style="font-size:12.800000190734863px">Cheers,</span></div><div><span style="font-size:12.800000190734863px">Lang.</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 29, 2016 at 4:17 PM, Sean Ogden <span dir="ltr"><<a href="mailto:sean@cs.cornell.edu" target="_blank">sean@cs.cornell.edu</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">It does work. I just tested it on my JIT. Thanks!<div><br></div><div>As for the part that couples them too tightly, would you recommend I just keep my own specialized version of CompileOnDemandLayer.h that includes this functionality, or do you have any ideas for a cleaner way to do this? I've noticed a couple of people asking for support for updating stub pointers for functions that are optimized at runtime, and I'd be willing to work on adding that support.</div></div><div><div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jul 29, 2016 at 6:53 PM Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Sean,<div><br></div><div>This is great, but it couples LogicalDylib too tightly to CompileOnDemandLayer. Does this alternative implementation of <span style="color:rgb(51,51,51);font-size:12.800000190734863px">getLogicalModuleResourcesForSy</span><font color="#333333"><span style="font-size:12.800000190734863px">mbol work for you (unfortunately I don't have a local test case for this yet):</span></font></div><div> <br></div><div></div></div><div dir="ltr"><div><div><font face="monospace, monospace"> LogicalModuleResources*</font></div><div><font face="monospace, monospace"> getLogicalModuleResourcesForSymbol(const std::string &Name,</font></div><div><font face="monospace, monospace"> bool ExportedSymbolsOnly) {</font></div></div></div><div dir="ltr"><div><div><font face="monospace, monospace"> for (auto LMI = LogicalModules.begin(), LME = LogicalModules.end();</font></div></div></div><div dir="ltr"><div><div><font face="monospace, monospace"> LMI != LME; ++LMI)</font></div><div><font face="monospace, monospace"> if (auto Sym = LMI->Resources.findSymbol(Name, ExportedSymbolsOnly))</font></div><div><font face="monospace, monospace"> return &LMI->Resources;</font></div><div><font face="monospace, monospace"> return nullptr;</font></div><div><font face="monospace, monospace"> }</font></div></div><div><br></div><div>If so I'd be happy to commit this on your behalf.</div><div><br></div><div>It might be nice to plumb support for this to the Orc C-bindings too: that would enable testing of this via the C-Bindings unit tests.</div><div><br></div><div>Thanks very much for working on this!</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 29, 2016 at 8:10 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><a href="mailto:lhames@gmail.com" target="_blank">+Lang Hames</a>, Master Regent of the Three <No, Two sir> JITs<br><br><div class="gmail_quote"><div><div><div dir="ltr">On Thu, Jul 28, 2016 at 12:31 PM Sean Ogden via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div dir="ltr">I needed to be able to update stub pointers for hot functions that get recompiled in a lazy JIT that uses CompileOnDemandLayer. In order to do this I added a method that allows pointers to be updated but does not expose any of the other internals of the COD layer.<div><br></div><div>Does anyone have a cleaner way to do this? Has something to facilitate this already been added? Would it be possible to merge this in?<br><div><br></div><div><div><font color="#333333">diff --git a/CompileOnDemandLayer.h b/CompileOnDemandLayer.h</font></div><div><font color="#333333">index bd192b8..4aa3362 100644</font></div><div><font color="#333333">--- a/CompileOnDemandLayer.h</font></div><div><font color="#333333">+++ b/CompileOnDemandLayer.h</font></div><div><font color="#333333">@@ -429,6 +429,28 @@ private:</font></div><div><font color="#333333"> return CalledAddr;</font></div><div><font color="#333333"> }</font></div><div><font color="#333333"><br></font></div><div><font color="#333333">+public:</font></div><div><font color="#333333">+ TargetAddress updatePointer(std::string FuncName, TargetAddress FnBodyAddr) {</font></div><div><font color="#333333">+ //Find out which logical dylib contains our symbol</font></div><div><font color="#333333">+ auto LDI = LogicalDylibs.begin();</font></div><div><font color="#333333">+ for (auto LDE = LogicalDylibs.end(); LDI != LDE; ++LDI) {</font></div><div><font color="#333333">+ if (auto LMResources = LDI->getLogicalModuleResourcesForSymbol(FuncName, false)) {</font></div><div><font color="#333333">+ Module &SrcM = LMResources->SourceModule->getResource();</font></div><div><font color="#333333">+ std::string CalledFnName = mangle(FuncName, SrcM.getDataLayout());</font></div><div><font color="#333333">+ if (auto EC = LMResources->StubsMgr->updatePointer(CalledFnName, FnBodyAddr)) {</font></div><div><font color="#333333">+ return 0;</font></div><div><font color="#333333">+ }</font></div><div><font color="#333333">+ else</font></div><div><font color="#333333">+ return FnBodyAddr;</font></div><div><font color="#333333">+ }</font></div><div><font color="#333333">+ }</font></div><div><font color="#333333">+ return 0;</font></div><div><font color="#333333">+ }</font></div><div><font color="#333333">+</font></div><div><font color="#333333">+private:</font></div></div><div><font color="#333333"><br></font></div><div><font color="#333333"><br></font></div><div><font color="#333333"><div>diff --git a/LogicalDylib.h b/LogicalDylib.h</div><div>index 84e3bf5..2e35cfd 100644</div><div>--- a/LogicalDylib.h</div><div>+++ b/LogicalDylib.h</div><div>@@ -17,6 +17,8 @@</div><div> #include "JITSymbol.h"</div><div> #include <string></div><div> #include <vector></div><div>+#include "llvm/IR/Mangler.h"</div><div>+#include "llvm/Support/raw_ostream.h"</div><div><br></div><div> namespace llvm {</div><div> namespace orc {</div><div>@@ -77,6 +79,24 @@ public:</div><div> return LMH->Resources;</div><div> }</div><div><br></div><div>+ static std::string mangle(StringRef Name, const DataLayout &DL) {</div><div>+ std::string MangledName;</div><div>+ {</div><div>+ raw_string_ostream MangledNameStream(MangledName);</div><div>+ Mangler::getNameWithPrefix(MangledNameStream, Name, DL);</div><div>+ }</div><div>+ return MangledName;</div><div>+ }</div><div>+</div><div>+ LogicalModuleResources* getLogicalModuleResourcesForSymbol(const std::string &Name, bool ExportedSymbolsOnly) {</div><div>+ for (auto LMI = LogicalModules.begin(), LME = LogicalModules.end();</div><div>+ LMI != LME; ++LMI)</div><div>+ if (auto Sym = findSymbolInLogicalModule(LMI, mangle(Name,LMI->Resources.SourceModule->getResource().getDataLayout()), ExportedSymbolsOnly))</div><div>+ return &LMI->Resources;</div><div>+ return nullptr;</div><div>+ }</div><div>+</div><div>+</div><div><br></div><div><br></div><div><br></div></font></div><div><br></div></div></div></div></div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>
</blockquote></div><br></div>
</blockquote></div>
</div></div></blockquote></div><br></div>
</blockquote></div>