[SystemZ] Use hasAddressTaken() with verifyNarrowIntegerArgs (NFC). #131039
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s() (NFC). Use hasAddressTaken() in SystemZ instead of doing this computation in isFullyInternal(), and make sure to only do this once per Function.
Member
|
@llvm/pr-subscribers-backend-systemz Author: Jonas Paulsson (JonPsson1) ChangesUse hasAddressTaken() in SystemZ instead of doing this computation in isFullyInternal(), and make sure to only do this once per Function. I started out by adding this cache in Function, but then with a test failure I realized that an optimization pass can of course change if a function has its address taken or not. Therefore it seems like the cache for this must be local to SystemZISelLowering. Full diff: https://github.com/llvm/llvm-project/pull/131039.diff 2 Files Affected:
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
index 1e0c303a2e4da..8e61e6bcda57e 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
@@ -10200,22 +10200,6 @@ 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 {
- if (!Fn->hasLocalLinkage())
- return false;
- for (const User *U : Fn->users()) {
- if (auto *CB = dyn_cast<CallBase>(U)) {
- if (CB->getCalledFunction() != Fn)
- return false;
- } else
- return false;
- }
- return true;
-}
-
static void printFunctionArgExts(const Function *F, raw_fd_ostream &OS) {
FunctionType *FT = F->getFunctionType();
const AttributeList &Attrs = F->getAttributes();
@@ -10234,6 +10218,16 @@ static void printFunctionArgExts(const Function *F, raw_fd_ostream &OS) {
OS << ")\n";
}
+bool SystemZTargetLowering::isInternal(const Function *Fn) const {
+ std::map<const Function *, bool>::iterator Itr = IsInternalCache.find(Fn);
+ if (Itr == IsInternalCache.end())
+ Itr = IsInternalCache
+ .insert(std::pair<const Function *, bool>(
+ Fn, (Fn->hasLocalLinkage() && !Fn->hasAddressTaken())))
+ .first;
+ return Itr->second;
+}
+
void SystemZTargetLowering::
verifyNarrowIntegerArgs_Call(const SmallVectorImpl<ISD::OutputArg> &Outs,
const Function *F, SDValue Callee) const {
@@ -10246,8 +10240,8 @@ verifyNarrowIntegerArgs_Call(const SmallVectorImpl<ISD::OutputArg> &Outs,
const Function *CalleeFn = nullptr;
if (auto *G = dyn_cast<GlobalAddressSDNode>(Callee))
if ((CalleeFn = dyn_cast<Function>(G->getGlobal())))
- IsInternal = isFullyInternal(CalleeFn);
- if (!verifyNarrowIntegerArgs(Outs, IsInternal)) {
+ IsInternal = isInternal(CalleeFn);
+ if (!IsInternal && !verifyNarrowIntegerArgs(Outs)) {
errs() << "ERROR: Missing extension attribute of passed "
<< "value in call to function:\n" << "Callee: ";
if (CalleeFn != nullptr)
@@ -10268,7 +10262,7 @@ verifyNarrowIntegerArgs_Ret(const SmallVectorImpl<ISD::OutputArg> &Outs,
if (!EnableIntArgExtCheck)
return;
- if (!verifyNarrowIntegerArgs(Outs, isFullyInternal(F))) {
+ if (!isInternal(F) && !verifyNarrowIntegerArgs(Outs)) {
errs() << "ERROR: Missing extension attribute of returned "
<< "value from function:\n";
printFunctionArgExts(F, errs());
@@ -10278,10 +10272,9 @@ verifyNarrowIntegerArgs_Ret(const SmallVectorImpl<ISD::OutputArg> &Outs,
// Verify that narrow integer arguments are extended as required by the ABI.
// Return false if an error is found.
-bool SystemZTargetLowering::
-verifyNarrowIntegerArgs(const SmallVectorImpl<ISD::OutputArg> &Outs,
- bool IsInternal) const {
- if (IsInternal || !Subtarget.isTargetELF())
+bool SystemZTargetLowering::verifyNarrowIntegerArgs(
+ const SmallVectorImpl<ISD::OutputArg> &Outs) const {
+ if (!Subtarget.isTargetELF())
return true;
if (EnableIntArgExtCheck.getNumOccurrences()) {
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.h b/llvm/lib/Target/SystemZ/SystemZISelLowering.h
index 839a550012444..971a948c3c857 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.h
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.h
@@ -815,13 +815,17 @@ class SystemZTargetLowering : public TargetLowering {
getTargetMMOFlags(const Instruction &I) const override;
const TargetRegisterClass *getRepRegClassFor(MVT VT) const override;
- bool isFullyInternal(const Function *Fn) const;
+private:
+ bool isInternal(const Function *Fn) const;
+ mutable std::map<const Function *, bool> IsInternalCache;
void verifyNarrowIntegerArgs_Call(const SmallVectorImpl<ISD::OutputArg> &Outs,
const Function *F, SDValue Callee) const;
void verifyNarrowIntegerArgs_Ret(const SmallVectorImpl<ISD::OutputArg> &Outs,
const Function *F) const;
- bool verifyNarrowIntegerArgs(const SmallVectorImpl<ISD::OutputArg> &Outs,
- bool IsInternal) const;
+ bool
+ verifyNarrowIntegerArgs(const SmallVectorImpl<ISD::OutputArg> &Outs) const;
+
+public:
};
struct SystemZVectorConstantInfo {
|
Member
|
Instead of adding the new map, can't this bit of information be cached in SystemZMachineFunctionInfo? |
Contributor
Author
I think SystemZTargetLowering is created once per module, while SystemZMachineFunctionInfo is created for each Function. |
Member
Well yes, that's why it should be the better place to store per-Function data, right? |
Contributor
Author
Oh, I wasn't aware that the MachineFunctionInfo survives the compilation of the MF. In that case I agree it would make sense. |
Contributor
Author
|
Hmm, I am kind of stuck at the point of getting a MachineFunction from the given Function. It's common to do MF->getFunction() and get the Function, but there doesn't seem to be a way to do it the other way around. There is a MachineFunctionAnalysis that runs, but I am not sure if it creates an MF (and MFI) ahead of time for all Functions in the module, or if it does this when compilation of it begins. Of course, an MF with an MFI would have to be created ahead of time for this to work. I guess it might not be so bad to add a Function flag directly and set it at the point of instruction selection, when the Function/Module will not be modified anymore. But that is a bit more intrusive into common-code on the other hand. |
uweigand
approved these changes
Mar 21, 2025
Member
uweigand
left a comment
uweigand
left a comment
There was a problem hiding this comment.
Given the discussion above, I think just keeping this cache is probably the best solution after all, so let's go with it as is. LGTM!
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/18142 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/14709 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use hasAddressTaken() in SystemZ instead of doing this computation in isFullyInternal(), and make sure to only do this once per Function.
I started out by adding this cache in Function, but then with a test failure I realized that an optimization pass can of course change if a function has its address taken or not. Therefore it seems like the cache for this must be local to SystemZISelLowering.