[PATCH] D72486: Add ThinLtoJIT example

Sun Jan 26 11:37:27 PST 2020

sgraenitz marked 4 inline comments as done.
sgraenitz added a comment.

In D72486#1839833 <https://reviews.llvm.org/D72486#1839833>, @lhames wrote:

> LGTM! I really like this. I'm all for adding it as another example if you're happy to maintain it.
>
> The code could probably use some extra comments (including pointers to your talk), but I think that would be better dealt with in-tree.

Thanks! I have a few small patches (polishing and fixing some todos) that I'd like to add next week, and a larger one (use call-through stubs for lazy module loading) that I will probably add in a separate commit. And sure, happy to add more comments step by step as the overall design consolidates.

> I need to check this out and play around with it a bit to wrap my head around it, especially the performance tuning issues. This is very exciting though!

With the two last updates the code should build just fine from TOT. The tinyexpr example is a good starting point. It's small and I used it in my presentation. I split it up into multiple files in order to illustrate the principle with per-module lazy compilation (the original version has only two translation units): https://github.com/weliveindetail/tinyexpr

================
Comment at: llvm/examples/ThinLtoJIT/ThinLtoJIT.cpp:258
+          CompileThreads->async([MU, &JD]() { MU->doMaterialize(JD); });
+        }
+      });
----------------
lhames wrote:
> lhames wrote:
> > sgraenitz wrote:
> > > @lhames: It looks like the session lock is a major bottleneck and not dispatching "trivial" modules to threads seems to help reducing the pressure. Do you think that's reasonable?
> > > 
> > > Are MaterializationUnit types meant to be distinguishable by type? Would it benefit from `isa<>`/`cast<>` support?
> > > 
> > Yes! I ran in to the same problem when developing our demo for the 2018 developer’s meeting.
> > 
> > I posted https://reviews.llvm.org/D39111 as a solution to distinguishing MUs by type, but haven’t had time to land it yet. Maybe it’s time to start making some noise on that review again.
> Side note: The bottle neck here may not be the session lock. I thought that was the culprit for our 2018 dev-meeting demo too, but it was really the thread pool: trivial materializes couldn’t get a thread to run on at all, rather than running but blocking on the session lock. Could that be what’s happening to you too?
> 
> Either way: I think I should add some logging to runSessionLocked to track how long threads spend holding the session lock, and how long they spend blocked waiting on it.
> I posted https://reviews.llvm.org/D39111 as a solution to distinguishing MUs by type

I see your point, that it would be convenient for clients to distinguish out-of-tree MU types using the same mechanism. For the moment, however, I could imagine the regular LLVM style `isa` for built-in MU types to be just fine. In case more and more clients utilize MUs as extension points to ORC, this would serve as a good argument for your open-class-hierarchy proposal (in addition to streamlining the llvm::ErrorInfo mechanism). I will put the former on my todo list and come back to you with my findings in a separate review.

> trivial materializes couldn’t get a thread to run on at all, rather than running but blocking on the session lock. Could that be what’s happening to you too?

Possible. I will check that in more detail when going ahead with performance tuning. Thanks for the heads-up!

================
Comment at: llvm/examples/ThinLtoJIT/ThinLtoModuleIndex.cpp:54
+  if (VI.getSummaryList().size() > 1) {
+    LLVM_DEBUG(dbgs() << "SummaryList with multiple entries!\n");
+  }
----------------
tejohnson wrote:
> sgraenitz wrote:
> > @tejohnson In which cases does the summary list for a global value have more than 1 entry? Is it reasonable to always refer to the first one (while this is a work in progress)? I am currently limited to sources in C and never see it happen.
> In C you can presumably still get duplicates on symbols declared with __attribute__((weak)), a GNU extension supported by gcc and clang.
> 
> Additionally, you could also get duplicates when there are same-named local symbols in same-named source files in different subdirectories and the user has not compiled with enough distinguishing source path. E.g. assume you have local symbol foo defined in path1/bar.c and path2/bar.c, and the user has done the following:
> 
> ```
> $ cd path1
> $ clang -c -flto=thin bar.c
> $ cd ../path2
> $ clang -c -flto=thin bar.c
> ```
> 
> When you link those two bar.o together it will have two matching GUID since the globalized name for locals used to compute the GUID has the relative path (encoded in the module) prepended, and in this case that is both "." essentially. It happens, and we support this typically by looking for one in the same module as the caller:
> http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#216
> 
> or in the current module when doing some handling in the ThinLTO backend, e.g.:
> http://llvm-cs.pcc.me.uk/lib/Transforms/Utils/FunctionImportUtils.cpp#62
> 
> 
Ok, thanks for the explanation! I should check that all modules have unique paths upfront when building the index. Then I could turn the debug warning here into an assertion, saying that attribute((weak)) is not yet supported. I think that's fine for a first version and I can improve that once the overall design consolidates. Thanks for the pointers on how this is handled in other cases!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72486/new/

https://reviews.llvm.org/D72486