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

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Great, thanks! A few minor comments below. Looks like it needs a clang-format too.



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


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:318
                            bool AllowCallerSuperset) const {
     if (!AllowCallerSuperset)
+      return VectLibrary == CalleeTLI.VectLibrary &&
----------------
This is set via a flag called "inline-caller-superset-nobuiltin". Suggest changing the name to something like "inline-caller-superset-tli" to reflect new larger scope. Also add a check with that option to your new inline test case.


================
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++)
----------------
Why not just have "i < NumVecLibs"?


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:590
             AvailableArray);
-  VectorDescs = TLI.VectorDescs;
-  ScalarDescs = TLI.ScalarDescs;
+  for (unsigned i = 0; i < sizeof(TLI.VecLibDescs) / sizeof(TLI.VecLibDescs[0]);
+       i++)
----------------
ditto


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1574
+  case NumVecLibs:
     break;
   }
----------------
Should these two be llvm_unreachable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77632