[PATCH] D77632: [TLI] Per-function fveclib for math library used for vectorization

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:272
+    else
+      VectLibrary = Impl.ClVectorLibrary;
+
----------------
wenlei wrote:
> tejohnson wrote:
> > Suggest moving the implementation of this constructor to the .cpp file, in which case you can just set VectLibrary directly from ClVectorLibrary there and remove the member on the Impl object.
> There're utilities that use `TargetLibraryInfo`, but don't link with `TargetLibraryInfo.o`. And looking at `TargetLibraryInfo`, all of the functions are in this header, so I assumed it's intentional to keep this type self-contained in this header, as it's public API, which is why I add `ClVectorLibrary` to Impl to pass it back to `TargetLibraryInfo`. For `TargetLibraryInfoImpl`, it's ok to have the implementation outside of the header. I can give it a try if keeping the class implementation/definition self-contained in the header isn't important.  
I don't think there should be anything using TLI without linking with libAnalysis, which contains TargetLibraryInfo.o. I don't think it should be important to keep the implementation in the header, any more so than for other headers in the Analysis library.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77632