[PATCH] D17066: libLTO: add a ThinLTOCodeGenerator on the model of LTOCodeGenerator.

[Sergei Larin via llvm-commits]

slarin added inline comments.

================
Comment at: include/llvm-c/lto.h:736
@@ +735,3 @@
+ */
+extern void thinlto_codegen_add_cross_referenced_symbol(thinlto_code_gen_t cg,
+                                                        const char *name,
----------------
joker.eph wrote:
> slarin wrote:
> > Does this handles commons properly?
> Can you be more specific? I'm not sure what is specific with common on this aspect?
I probably not fully understand how this list should work... but this is not the proper place to figure it out - please ignore this comment for now.

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:148
@@ +147,3 @@
+  void setCodeGenOptLevel(CodeGenOpt::Level CGOptLevel) {
+    TMBuilder.CGOptLevel = CGOptLevel;
+  }
----------------
joker.eph wrote:
> slarin wrote:
> > Can we theoretically have a mixture of opt levels? -O2/-Os? Should I be able to respect per library opt level?
> Ideally I think we'd want this parameter to be recorded in the bitcode itself, what do you think?
> Right now we don't even expose any linker flag for that.
> 
> Here it is about the *codegen* opt level though, it won't impact the optimizer.
>>Ideally I think we'd want this parameter to be recorded in the bitcode itself, what do you think?

Yes. Certainly.

>> it is about the *codegen*

...yes. My target has codegen properties that are exposed to a user, which might produce drastically different results if not set properly, but once again, ideally it should come from bitcode.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:67
@@ +66,3 @@
+    ModuleOrErr = parseBitcodeFile(Buffer, Context);
+  }
+  if (std::error_code EC = ModuleOrErr.getError()) {
----------------
joker.eph wrote:
> slarin wrote:
> > I do not know if it is relevant at this point, but for what it worth - my IR might have metadata that changes opt/codegen.
> slarin: how does it play with cross-module importing? If a function is defined in a module compiled with O3 but imported in a module compiled with O2?
If I know settings for each module I can handle these situations in platform specific way. Besides rough optimization levels I have different addressing modes used in different modules, and I might chose not to mix certain features at all... At this point my main concern seems to be revolving around general LTO issues (like mixing different optimization scopes into one) and might not be "thin"-lto specific. We had to jump through hoops for regular LTO, and I see very similar set of issues being designed in here as well...  

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:81
@@ +80,3 @@
+  if (TempDir.empty())
+    return;
+  // User asked to save temps, let dump the bitcode file after import.
----------------
joker.eph wrote:
> slarin wrote:
> > Default to /tmp?
> The way the client is enabling dumping temporaries is by providing a path.
> Having a default would mean either always dumping temporaries (we don't want that...) or having a more complicated way of enabling dumping.
> 
I see...

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:184
@@ +183,3 @@
+  // Save temps: after IPO.
+  saveTempBitcode(TheModule, SaveTempsDir, count, ".1.IPO.bc");
+
----------------
joker.eph wrote:
> slarin wrote:
> > Sorry, I miss something - why is this unconditional?
> As mentioned above, this is conditional in the `saveTempBitcode` function itself, which starts with 
> 
> ```
> if (SaveTempsDir.empty()) return;
> ```
> 
> makes sense?
Totally


http://reviews.llvm.org/D17066