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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

New pass manager command line option missing.

FWIW, you can checkout D69930 <https://reviews.llvm.org/D69930> for an example of adding a pass with more hookup into the system.



================
Comment at: llvm/include/llvm/Transforms/Utils/InjectTLIMappings.h:22
+  static StringRef name() { return "InjectTLIMappings"; }
+  explicit InjectTLIMappings() {}
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
----------------
I doubt you need the constructor or the name function.


================
Comment at: llvm/include/llvm/Transforms/Utils/ModuleUtils.h:115
 namespace VFABI {
-
 /// \defgroup Vector Function ABI (VABI) Module functions.
----------------
Unrelated?


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:1
+//===- InjectTLIMappings.cpp - TODO description --------------------===//
+//
----------------
TODO, also below


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:19
+
+#define DEBUG_TYPE "inject-TLI-mappings"
+
----------------
Is there precedent for not having it in all lower-case letters?


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:74
+  if (Global)
+    return;
+
----------------
Wasn't that checked below but with a different call to the module?


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:80
+    Tys.push_back(ToVectorTy(ArgOperand->getType(), VF));
+  FunctionType *FTy = FunctionType::get(RetTy, Tys, /*isVarArg=*/false);
+  Function *VectorF =
----------------
Where is the VarArgs restrictions checked? If it is implicit, please add an assertion that the original callee is not vararg.


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:92
+    Global = M->getNamedValue(VFName);
+    assert(Global && "Missing function declaration.");
+    appendToCompilerUsed(*M, {Global});
----------------
Why do you need to query Global here? Global is VectorF isn't it?


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:118
+  const auto OriginalSetOfMappings =
+      SetVector<StringRef>(Mappings.begin(), Mappings.end());
+  // 16 is the max number of lanes the TLI has in its VecDesc
----------------
For brevity and without the need to assign:
`SetVector<StringRef> OriginalSetOfMappings(Mappings.begin(), Mappings.end());`



================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:133
+    }
+  }
+
----------------
Can we make 16 a return value of a (maybe static) function in TLI?

Style, above:
```
if (!...count(...))
```


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:152
+}
+} // End namespace llvm
+
----------------
I doubt you need the lllvm.

Above, `auto CI*`please :)


================
Comment at: llvm/test/Transforms/Util/add-TLI-mappings.ll:1
+; RUN: opt -vector-library=SVML -inject-TLI-mappings -S < %s | FileCheck %s
+
----------------
andwar wrote:
> For this to work you need to register a command line option. Why not use `print-after` and `print-before` instead? Or maybe we do need a command line option?
command line options are good for various things, please make sure they work. (new and old PM)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70107