[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

Tue May 14 13:33:40 PDT 2019

tejohnson marked 2 inline comments as done.
tejohnson added inline comments.

================
Comment at: clang/test/CodeGen/svml-calls.ll:16
+
+define void @sin_f64(double* nocapture %varray) {
+; CHECK-LABEL: @sin_f64(
----------------
steven_wu wrote:
> Personally, I think codegen tests like this will be cleaner to keep in LLVM. Clang tests just test the IRGen of the module flag and LLVM tests check that those flags are respected and module flag merge is respected.
Ok. I originally was doing it here to ensure that ThinLTO distributed backends (which use clang) are fixed. But now that the approach no longer involves passing down additional info via that path into LTO, I don't think we need to test this explicitly but rather just as an LLVM LTO test. Will move.

================
Comment at: llvm/lib/LTO/LTOBackend.cpp:221

+static TargetLibraryInfoImpl *createTLII(Module &Mod, TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =
----------------
steven_wu wrote:
> Should this be done not just for LTOBackend but for regular compilation as well? LegacyCodegenerator and llc can all be benefit from a matching TargetLibraryInfo?
Yeah, probably. I think the best way to have this utilized everywhere is to move the below code into the TargetLibraryInfoImpl itself - by having it also take the Module as a parameter). Probably as a required parameter, to ensure it is used consistently. WDYT? 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162