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

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Needs testing of the inline handling, and of LTO linking IR with different attributes (which is going to hit your assert, see below).



================
Comment at: clang/lib/CodeGen/CGCall.cpp:1987
 
+  // Attach "vect-lib" attribute to function based on '-fveclib' setting.
+  addVectLibAttributes(FuncAttrs, getCodeGenOpts());
----------------
Nit: why not "vec-lib" or just "veclib", to match the option?


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:47
 /// depends on the triple. So users typically interact with the \c
 /// TargetLibraryInfo wrapper below.
 class TargetLibraryInfoImpl {
----------------
Key comment about handling of TLII vs TLI. The former is computed once per module by the analysis (which is going to be the combined module in the case of LTO), the latter is the per-function data structure.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:91
+  /// Vector library available for vectorization.
+  VectorLibrary VectLibrary = NoLibrary;
 
----------------
To avoid building and storing the VectorDescs and ScalarDescs for every function in the TLI, what I would do is keep 3 sets of VectorDescs/ScalarDescs on the TLII object (one set per possible veclib, built once per module during construction of the TLII), then move the new VectorLibrary member to the TLI and set it there per function based on the attribute, and use it to select which pair of VectorDescs/ScalarDescs is queried.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:275
     if (!AllowCallerSuperset)
-      return OverrideAsUnavailable == CalleeTLI.OverrideAsUnavailable;
+      return Impl->VectLibrary == CalleeTLI.Impl->VectLibrary &&
+             OverrideAsUnavailable == CalleeTLI.OverrideAsUnavailable;
----------------
I don't think this will do anything currently since the TLII is built once per module by the analysis. You'll hit your assert about incompatibility below first, see comment there.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1549
+    assert(VectLibrary == VecLib && 
+           "Conflicting VectorLibrary detected");
+    return;
----------------
You'll certainly hit this assert if you try LTO linking two .ll files built with different -fveclib options, because the TLII is built once per module by the analysis.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1631
+  if (!VectLibName.empty())
+    BaselineInfoImpl->addVectorizableFunctionsFromVecLib(VectLibName);
+
----------------
This is going to override the baseline TLI veclib with whatever is the latest function we build a TLI for (and you'll hit the assert as noted earlier if they conflict).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77632