[PATCH] D91250: Support intrinsic overloading on unnamed types
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 10 02:49:02 PST 2021
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
More information about the llvm-commits
mailing list