[PATCH] D91250: Support intrinsic overloading on unnamed types

[Florian Hahn via Phabricator via llvm-commits]

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

LGTM, thanks! This patch should address the main issue earlier patches had, which were not handling name clashes on combining modules properly. Please wait a couple of days with committing, in cases there are any other comments.



================
Comment at: llvm/docs/LangRef.rst:11516
+``llvm.ssa.copy.p0s_s.2(%42*)``) The number is tracked in the LLVM module and
+it ensures unique names in a module. While linking together two modules, it is
+still possible to get a name clash. In that case one of the names will be
----------------
nit: in the module?


================
Comment at: llvm/docs/LangRef.rst:11518
+still possible to get a name clash. In that case one of the names will be
+changed.
+
----------------
nit: changed how? new numbers will get assigned, right?


================
Comment at: llvm/include/llvm/IR/Module.h:202
+      CurrentIntrinsicIds; ///< Keep track of the current unique id count for
+                           ///< the specified intrinsic basename
+  DenseMap<std::pair<Intrinsic::ID, const FunctionType *>, unsigned>
----------------
jeroen.dobbelaere wrote:
> fhahn wrote:
> > nit: period at end.
> > nit: period at end.
> 
> What is the preference ? the comments before are also mixed wrt to the period.
My preference is `.` at the end, but I am not sure if there's any rule.


================
Comment at: llvm/include/llvm/IR/Module.h:204
+  DenseMap<std::pair<Intrinsic::ID, const FunctionType *>, unsigned>
+      UniquedIntrinsicNames; ///< Keep track of uniqued names of intrinsics
+                             ///< based on unnamed types
----------------
jeroen.dobbelaere wrote:
> fhahn wrote:
> > Where those the name come in here? It maps intrinsic ids to other ids?
> > Where those the name come in here? It maps intrinsic ids to other ids?
> 
> This map, based on the intrinsic ID and the FunctionType, allow us to remember what extension number has been associated to that combination. The uniqued name is based on the intrinsic ID, the FunctionType and that extension number.
> 
Sorry, I meant the `Name` part in the name of the variable `UniquedIntrinsicNames`. The variable does not contain the full names, just the ids for intrinsic::id, type combinations. Not sure if there's a better name.


================
Comment at: llvm/lib/IR/Function.cpp:809
+std::string Intrinsic::getName(ID id, ArrayRef<Type *> Tys) {
+  return getName(id, Tys, nullptr, nullptr);
+}
----------------
jeroen.dobbelaere wrote:
> fhahn wrote:
> > can we assert that there are no unnamed types involved here?
> Should be possible. I would add the assert at line 796-797.
Sounds good.


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/pr48340.ll:1
+; RUN: opt -loop-vectorize -S -o - < %s | FileCheck %s
+
----------------
can you add `-force-vector-width=4 -force-vector-interleave=1` or some similar settings? That way the test should be more robust with respect to cost-model changes.


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/pr48340.ll:55
+attributes #0 = { "target-cpu"="skylake" }
+
----------------
nit: stray newline.


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

https://reviews.llvm.org/D91250