[PATCH] D91250: Support intrinsic overloading on unnamed types

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the updates. I think this basically looks good now, just another round of smallish comments.



================
Comment at: llvm/docs/LangRef.rst:11513
+that depend on an unnamed type in one of its overloaded argument types get an
+additional ``.<number>`` suffix, that allows to differentiate such intrinsics.
+(For example: ``llvm.ssa.copy.p0s_s.2(%42*)``) The number is tracked in the
----------------
nit: Perhaps break up the sentence and mention that it allows differentiating intrinsics that have different unnamed types as arguments?


================
Comment at: llvm/include/llvm/IR/Intrinsics.h:69
+  /// overloads are required, it is safe to use this version, but better to use
+  /// the StringRef version. The function type can be provided as a speedup.
+  /// It is used (or computed) if one of the types is based on an unnamed type.
----------------
nit: Perhaps it's simpler to just say something like `a function type \p FT can be provided to avoid computing it.`.


================
Comment at: llvm/include/llvm/IR/Intrinsics.h:72
+  std::string getName(ID id, ArrayRef<Type *> Tys, Module *M,
+                      llvm::FunctionType *FT);
 
----------------
nit: `llvm::` should not be needed.


================
Comment at: llvm/lib/IR/Function.cpp:726
+/// indicating that extra care must be taken to ensure a unique name.
+static std::string getMangledTypeStr(Type *Ty, bool &HasUnnamedType) {
   std::string Result;
----------------
nit: I think it would be more explicit to return a pair with a bool, rather than having this out parameter.  But. I guess the implementation would get a bit messier in the function.


================
Comment at: llvm/lib/IR/Function.cpp:803
+  if (M && HasUnnamedType) {
+    if (FT == nullptr)
+      FT = getType(M->getContext(), id, Tys);
----------------
nit: checking explicitly for `== nullptr` is very uncommon in LLVM, can you just check for `FT`/`!FT`?


================
Comment at: llvm/lib/IR/Module.cpp:478
+                                           const FunctionType *Proto) {
+  auto encode = [&](unsigned Suffix) {
+    return (Twine(BaseName) + "." + Twine(Suffix)).str();
----------------
nit: no need to capture everything, it should be enough to capture `BaseName`?

Also, please fix the tidy warning.


================
Comment at: llvm/lib/IR/Module.cpp:502
+    GlobalValue *F = getNamedValue(NewName);
+    if (F == nullptr) {
+      // Reserve this entry for the new proto
----------------
nit: uncommon in LLVM to check `== nullptr`.


================
Comment at: llvm/test/Transforms/LoopVectorize/pr48340.ll:3
+
+; ModuleID = 'a.ll'
+source_filename = "a.6dqf8b0y-cgu.0"
----------------
not needed.


================
Comment at: llvm/test/Transforms/LoopVectorize/pr48340.ll:4
+; ModuleID = 'a.ll'
+source_filename = "a.6dqf8b0y-cgu.0"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
----------------
not needed.


================
Comment at: llvm/test/Transforms/LoopVectorize/pr48340.ll:6
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
Is this required to the test? If so, please move in the X86 subdirectory.


================
Comment at: llvm/test/Transforms/LoopVectorize/pr48340.ll:13
+; CHECK-LABEL: @foo(
+; CHECK:         [[WIDE_MASKED_GATHER:%.*]] = call <4 x %0*> @llvm.masked.gather.v4p0s_s.v4p0p0s_s.0(<4 x %0**> [[TMP5:%.*]], i32 8, <4 x i1> <i1 true, i1 true, i1 true, i1 true>, <4 x %0*> undef)
+; CHECK-NEXT:    [[WIDE_MASKED_GATHER1:%.*]] = call <4 x %0*> @llvm.masked.gather.v4p0s_s.v4p0p0s_s.0(<4 x %0**> [[TMP6:%.*]], i32 8, <4 x i1> <i1 true, i1 true, i1 true, i1 true>, <4 x %0*> undef)
----------------
You could also check for the `vector.body:` block.


================
Comment at: llvm/test/Transforms/LoopVectorize/pr48340.ll:20
+1:                                                ; preds = %1, %0
+  %2 = phi i64* [ undef, %0 ], [ %3, %1 ]
+  %3 = getelementptr inbounds i64, i64* %2, i64 2
----------------
is it possible to get rid for the undefs in the tests? (and also have named values for the basic blocks at least?


================
Comment at: llvm/test/Transforms/LoopVectorize/pr48340.ll:53
+
+!llvm.module.flags = !{!0}
+
----------------
is this needed?


================
Comment at: llvm/test/Transforms/LoopVectorize/pr48340.ll:55
+
+!0 = !{i32 2, !"RtLibUseGOT", i32 1}
----------------
is this needed?


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

https://reviews.llvm.org/D91250