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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/ModuleUtils.h:115
 namespace VFABI {
-
 /// \defgroup Vector Function ABI (VABI) Module functions.
----------------
jdoerfert wrote:
> Unrelated?
Yes, but still useful. I have also removed a group comment.

I'd rather not create a separate patch for this?


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:19
+
+#define DEBUG_TYPE "inject-TLI-mappings"
+
----------------
jdoerfert wrote:
> Is there precedent for not having it in all lower-case letters?
The pass debug option is all lower case now (and consequently, the command line too).


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:74
+  if (Global)
+    return;
+
----------------
jdoerfert wrote:
> Wasn't that checked below but with a different call to the module?
This avoids running the creation of the function if the function already exists. Otherwise I expect `Function::Create` to have some problems.


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


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:152
+}
+} // End namespace llvm
+
----------------
jdoerfert wrote:
> I doubt you need the lllvm.
> 
> Above, `auto CI*`please :)
> I doubt you need the lllvm.

You mean I don't need to wrap the pass in the llvm namespace? I have done it in the header file too. Is that wrong?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70107