[SystemZ, ArgExt verification] Cache results of isFullyInternal(). #130693
Conversation
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-backend-systemz Author: Jonas Paulsson (JonPsson1) ChangesIt has found to be quite a slowdown to traverse the users of a function from each call site when it is called many (~70k) times. This is my first attempt that works as expected, but not sure if it's desired to introduce the new Function member. As this is at the I/R level, it is not strictly of SystemZ interest only, although at the moment it is. isFullyInternal() could be moved into Function perhaps if some other target decides to do a similar check, so in a sense it belongs here. There is the IsNewDbgInfoFormat which is also kind of a flag without being a proper attribute, which would be overkill in this case. Maybe an alternative might be to have a set of Function pointers in SystemZIselLowering that should hopefully live throughout the compilation of the module, which is what is needed. It seems safer though to avoid the potential reuse of pointers entirely. Not sure if MachineModuleInfo could be used - do not see other similar usages. It is probably also a good idea to avoid calling isFullyInternal() unless the actual check is enabled. Waiting with that until the test case is confirmed to be resolved. Full diff: https://github.com/llvm/llvm-project/pull/130693.diff 4 Files Affected:
diff --git a/llvm/include/llvm/IR/Function.h b/llvm/include/llvm/IR/Function.h
index 29041688124bc..09ea8417819de 100644
--- a/llvm/include/llvm/IR/Function.h
+++ b/llvm/include/llvm/IR/Function.h
@@ -115,6 +115,13 @@ class LLVM_ABI Function : public GlobalObject, public ilist_node<Function> {
/// \ref BasicBlock.
bool IsNewDbgInfoFormat;
+ /// If this function is known to be fully internal or not, this is set. A
+ /// function with local linkage can still be passed as a pointer to an
+ /// external routine, in which case it would be classified as External
+ /// here.
+ enum class FunctionVisibility { Unknown, FullyInternal, External };
+ mutable FunctionVisibility FunVisibility;
+
/// hasLazyArguments/CheckLazyArguments - The argument list of a function is
/// built on demand, so that the list isn't allocated until the first client
/// needs it. The hasLazyArguments predicate returns true if the arg list
diff --git a/llvm/lib/IR/Function.cpp b/llvm/lib/IR/Function.cpp
index 5666f0a53866f..ff175027c0e75 100644
--- a/llvm/lib/IR/Function.cpp
+++ b/llvm/lib/IR/Function.cpp
@@ -492,7 +492,8 @@ Function::Function(FunctionType *Ty, LinkageTypes Linkage, unsigned AddrSpace,
const Twine &name, Module *ParentModule)
: GlobalObject(Ty, Value::FunctionVal, AllocMarker, Linkage, name,
computeAddrSpace(AddrSpace, ParentModule)),
- NumArgs(Ty->getNumParams()), IsNewDbgInfoFormat(UseNewDbgInfoFormat) {
+ NumArgs(Ty->getNumParams()), IsNewDbgInfoFormat(UseNewDbgInfoFormat),
+ FunVisibility(FunctionVisibility::Unknown) {
assert(FunctionType::isValidReturnType(getReturnType()) &&
"invalid return type");
setGlobalObjectSubClassData(0);
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
index bb584b7bb5c9a..a26e1d7b80628 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
@@ -10200,10 +10200,7 @@ SDValue SystemZTargetLowering::lowerVECREDUCE_ADD(SDValue Op,
DAG.getConstant(OpVT.getVectorNumElements() - 1, DL, MVT::i32));
}
-// Only consider a function fully internal as long as it has local linkage
-// and is not used in any other way than acting as the called function at
-// call sites.
-bool SystemZTargetLowering::isFullyInternal(const Function *Fn) const {
+bool SystemZTargetLowering::isFullyInternal_impl(const Function *Fn) const {
if (!Fn->hasLocalLinkage())
return false;
for (const User *U : Fn->users()) {
@@ -10216,6 +10213,17 @@ bool SystemZTargetLowering::isFullyInternal(const Function *Fn) const {
return true;
}
+// Only consider a function fully internal as long as it has local linkage
+// and is not used in any other way than acting as the called function at
+// call sites.
+bool SystemZTargetLowering::isFullyInternal(const Function *Fn) const {
+ if (Fn->FunVisibility == Function::FunctionVisibility::Unknown)
+ Fn->FunVisibility = isFullyInternal_impl(Fn)
+ ? Function::FunctionVisibility::FullyInternal
+ : Function::FunctionVisibility::External;
+ return Fn->FunVisibility == Function::FunctionVisibility::FullyInternal;
+}
+
static void printFunctionArgExts(const Function *F, raw_fd_ostream &OS) {
FunctionType *FT = F->getFunctionType();
const AttributeList &Attrs = F->getAttributes();
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.h b/llvm/lib/Target/SystemZ/SystemZISelLowering.h
index 839a550012444..d920915f5083a 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.h
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.h
@@ -815,6 +815,7 @@ class SystemZTargetLowering : public TargetLowering {
getTargetMMOFlags(const Instruction &I) const override;
const TargetRegisterClass *getRepRegClassFor(MVT VT) const override;
+ bool isFullyInternal_impl(const Function *Fn) const;
bool isFullyInternal(const Function *Fn) const;
void verifyNarrowIntegerArgs_Call(const SmallVectorImpl<ISD::OutputArg> &Outs,
const Function *F, SDValue Callee) const;
|
|
In the end, the information we need is the same that is already computed by So I think in the long run it would be best for the SystemZ back-end to simply use |
|
/cherry-pick 378739f |
|
/pull-request #130998 |
…l(). (llvm#130693) It has found to be quite a slowdown to traverse the users of a function from each call site when it is called many (~70k) times. This patch fixes this for now as long as this verification is disabled by default, but there is still a need to eventually cache the results to avoid recomputation. Fixes llvm#130541 (cherry picked from commit 378739f)
…l(). (llvm#130693) It has found to be quite a slowdown to traverse the users of a function from each call site when it is called many (~70k) times. This patch fixes this for now as long as this verification is disabled by default, but there is still a need to eventually cache the results to avoid recomputation. Fixes llvm#130541
It has found to be quite a slowdown to traverse the users of a function from each call site when it is called many (~70k) times.
Make sure to only do this once per function by using a new Function flag for this purpose.
This is my first attempt that works as expected, but not sure if it's desired to introduce the new Function member. As this is at the I/R level, it is not strictly of SystemZ interest only, although at the moment it is. isFullyInternal() could be moved into Function perhaps if some other target decides to do a similar check, so in a sense it belongs here.
There is the IsNewDbgInfoFormat which is also kind of a flag without being a proper attribute, which would be overkill in this case.
Maybe an alternative might be to have a set of Function pointers in SystemZIselLowering that should hopefully live throughout the compilation of the module, which is what is needed. It seems safer though to avoid the potential reuse of pointers entirely.
Not sure if MachineModuleInfo could be used - do not see other similar usages.
It is probably also a good idea to avoid calling isFullyInternal() unless the actual check is enabled. Waiting with that until the test case is confirmed to be resolved.