[PATCH] D45308: [IPRA] Do not collect register usage information on functions that can be derefined

[Kit Barton via Phabricator via llvm-commits]

kbarton added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetFrameLowering.h:338
   static bool isSafeForNoCSROpt(const Function &F) {
-    if (!F.hasLocalLinkage() || F.hasAddressTaken() ||
+    if (!F.isDefinitionExact() || F.hasAddressTaken() ||
         !F.hasFnAttribute(Attribute::NoRecurse))
----------------
MatzeB wrote:
> MatzeB wrote:
> > I believe this should rather be
> > `F.hasAvailableExternallyLinkage() || F.isInterposable() || F.hasAddressTaken() || !F.hasFnAttribute(Attribute::NoRecurse)`.
> And we probably should also abort for `isLinkOnceLinkage()` (though I'm currently not sure if that linkage is even possible when the symbol doesn't have an external linkage...)
The implementation of isDefinitionExact calls mayBeDerefined, which checks for both hasAvailableExternallyLinkage, and isInterposable. It also checks for many other linkage types as well. 

Was your suggestion to relax the check to make this work on the other linkage types that would currently be prevented by the call to isDefinitonExact? If not, I would prefer to keep the call to isDefinitionExact as I think it's more concise. 


================
Comment at: llvm/lib/CodeGen/RegUsageInfoCollector.cpp:104-105
 
+ if (!F.isDefinitionExact())
+    return false;
+
----------------
MatzeB wrote:
> Maybe add a comment that this is a shortcut as the result won't get used?
I've been talking with Vivek on how we can change this. There is a subtle handshake between IPRA and isSafeForNoCSROpt that I'd like to make more explicit. I'll be posting another patch soon, as soon as I've had a chance to redo this. If that doesn't get accepted, then yes, I'll definitely add a comment here.


https://reviews.llvm.org/D45308