[PATCH] D70107: [VFABI] TargetLibraryInfo mappings in IR.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

Some minor comments below, otherwise LGTM.

@sdesmalen any more comments?



================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:120
+  //  All VFs in the TLI are powers of 2.
+  for (unsigned VF = 2; VF <= TLI.getWidestVF(ScalarName); VF *= 2) {
+    const std::string TLIName = TLI.getVectorizedFunction(ScalarName, VF);
----------------
The call is not free but invariant:
`for (unsigned VF = 2, MaxVF = TLI.getWidestVF(ScalarName); VF <= MaxVF; VF *= 2) {`


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:154
+  const bool Changed = runImpl(TLI, F);
+  (void)Changed;
+  // Even if the pass adds IR attributes, the analyses are preserved.
----------------
Remove `Changed` or doe something with it.


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:170
+  AU.addRequired<TargetLibraryInfoWrapperPass>();
+  AU.addPreserved<TargetLibraryInfoWrapperPass>();
+}
----------------
You can preserve more here, e.g. all CFG analysis.


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:182
+INITIALIZE_PASS_END(InjectTLIMappingsLegacy, DEBUG_TYPE, "Inject TLI Mappings",
+                    false, false)
+
----------------
I think one of the false could be a true, unclear if that will make a difference though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70107