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

[Wenlei He via Phabricator via cfe-commits]

wenlei marked 4 inline comments as done.
wenlei added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:272
+    else
+      VectLibrary = Impl.ClVectorLibrary;
+
----------------
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.  


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:577
   memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
-  VectorDescs = TLI.VectorDescs;
-  ScalarDescs = TLI.ScalarDescs;
+  for (unsigned i = 0; i < sizeof(TLI.VecLibDescs) / sizeof(TLI.VecLibDescs[0]);
+       i++)
----------------
tejohnson wrote:
> Why not just have "i < NumVecLibs"?
Good catch, thanks..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77632