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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/ModuleUtils.h:115
 namespace VFABI {
-
 /// \defgroup Vector Function ABI (VABI) Module functions.
----------------
fpetrogalli wrote:
> jdoerfert wrote:
> > Unrelated?
> Yes, but still useful. I have also removed a group comment.
> 
> I'd rather not create a separate patch for this?
create an RFC one and commit it w/o review if it is trivial.


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:74
+  if (Global)
+    return;
+
----------------
fpetrogalli wrote:
> 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.
I mean you checked that already:
```
      Function *VariantF = M->getFunction(TLIName);
      if (!VariantF)
        addVariantDeclaration(CI, VF, TLIName);
```

but once with `getFunction` and once with `getNamedValue`. I think you don't need two checks and you should pick consistent the one lookup call you want.


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


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:152
+}
+} // End namespace llvm
+
----------------
fpetrogalli wrote:
> 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?
you don't need it here because you opened the namespace. you should not open namespaces in headers that is why you wrap it in the namespace there. Plus, you don't need the explicit qualification here because these are not top level declarations as you have in the header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70107