[PATCH] D51744: [WIP] Early ThinLTOLayer2 prototype

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added inline comments.


================
Comment at: unittests/ExecutionEngine/Orc/Inputs/Bar.ll:22
+^0 = module: (path: "Bar.o", hash: (1372045330, 2056102272, 1752332163, 195788812, 3147837727))
+^1 = gv: (name: "bar", summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 1, live: 0, dsoLocal: 0), insts: 1))) ; guid = 16434608426314478903
----------------
tejohnson wrote:
> sgraenitz wrote:
> > `notEligibleToImport: 1` popped up recently and currently fails discovery with: //ignored! No qualifying callee with summary found.// Didn't find a quick way to fix that, so for now I will just clear it manually. Generated with:
> > ```
> > clang -c -flto=thin Foo.cpp -o - | llvm-dis -o Foo.ll
> > ```
> There are 2 categories that will cause notEligibleToImport to be set:
> 1) Correctness: the value is a local used in some opaque way (e.g. in module asm or on the llvm.used) that means the backend cannot safely promote it to global scope and rename.
> 2) Profitability: if we think the backend cannot inline it anyway, then there has traditionally not been a reason to import.
> 
> Looks like you are being hit here by reason 2 - since bar has a "noinline" attribute.
> 
> I've been thinking about splitting these categories into 2 different flags, looks like that might be useful for you.
Thanks for the clarification. IIUC it's a hint for the importer to ignore the function for an aggregate of reasons summaries don't otherwise provide. So it's only a matter of interpretation, in the way that the summary generator may decide to set notEligibleToImport on a GV, but that doesn't cause it to omit regular information in the summary, right?

> I've been thinking about splitting these categories into 2 different flags, looks like that might be useful for you.

Well, yes greater detail usually allows for better reasoning. But it sounds to me like both categories don't affect pure discovery the same way they affect actual cross-module-import. After all I only use the summaries to select which modules to compile at which time. Function import and inlining will certainly be interesting at some point, but it feels like a distance future.

So far I just used `computeImportForFunction()` and it's fine for my simple examples. I guess discovery in real-world code would profit from an adapted version that interprets summary information slightly different (will probably need that anyway for configurability).

Again, thank you so much for making the time and sharing your knowledge!


Repository:
  rL LLVM

https://reviews.llvm.org/D51744