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

Francesco Petrogalli via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 14 20:37:46 PST 2019


fpetrogalli added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/InjectTLIMappings.h:22
+  static StringRef name() { return "InjectTLIMappings"; }
+  explicit InjectTLIMappings() {}
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
----------------
andwar wrote:
> jdoerfert wrote:
> > I doubt you need the constructor or the name function.
> What about this one? You don't need the `name` function, the one provided by the base class is fine: https://github.com/llvm/llvm-project/blob/848007cfbc7509543c5b8604ae063bb6c8ffa0a9/llvm/include/llvm/IR/PassManager.h#L373
> 
> As for the constructor, the auto-generated one should be sufficient here. Is it not?
Contructor and name function removed. That was too much copy and paste.


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:74
+  if (Global)
+    return;
+
----------------
jdoerfert wrote:
> 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.
I was using `Global` mostly to be able to call `appendToCompilerUsed`, which works on `global`s. Then, I realized that `Function` inherits from `Global`, so there is no need to use `Global` at all.

I also have replaced the check on having an empty body with an assertion, just in case someone modifies the function in the middle and populates the body of the function before invoking `appendToCompilerUsed`.


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:92
+    Global = M->getNamedValue(VFName);
+    assert(Global && "Missing function declaration.");
+    appendToCompilerUsed(*M, {Global});
----------------
jdoerfert wrote:
> 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?
Not it is fixed, no more `Global`.


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:152
+}
+} // End namespace llvm
+
----------------
jdoerfert wrote:
> 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.
Facepalm myself at the `using namespace llvm;` on top of this cpp file.

Thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70107





More information about the llvm-commits mailing list