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

[kbarton]

Summary

Do not collect register usage information to be used by IPRA on functions that can be derefined.
If it is possible for a function to be replaced at link time with another version of the function, that has been optimized differently, we cannot assume the reg mask will be correct. For a more indepth explanation of de-refinement, see https://reviews.llvm.org/D18634.

This patch checks the isDefinitionExact method on the function to ensure the function cannot be redefined. If the definition is not exact, do not refine the register usage information during the collection phase of IPRA.
The isSafeForNoCSROpt method is also updated to ensure the function cannot be redefined. This is necessary because the isSafeForNoCSROpt relies on IPRA information, and if IPRA cannot be performed on a function, it consequently is not safe for NoCSROpt.

[nemanjai]

This seems perfectly fine. Maybe the test case can be slightly improved. Adding the originator of this code to the review.

llvm/test/CodeGen/PowerPC/ipra-odr.ll

2

Can we make the checks stronger with -print-regusage?

12

I imagine this stray b character will cause test case failures.

[vivekvpandya]

Change looks good to me. But please fix the new test case as suggested by @nemanjai
In this new test do you see improvement in register allocation due to IPRA? If test-case does not show any benefit due to IPRA then it is not suitable test for this patch.
Also I hope this patch is also tested with other architecture which have LIT based test-cases for IPRA. It is very unlikely that such test-case will have function with linkage type which return true for isInterposableLinkage() but if such case is there then this change may break such tests.

[jonpa]

ping!

I recently tried IPRA on SystemZ, and tracked down a benchmark crash to this very same issue, see https://bugs.llvm.org/show_bug.cgi?id=37284. I also verified that this patch seems to handle my test case.

[jonpa]

With this patch, any called function which is not "definition exact" keeps the unmodified regmask on the call instruction, which assumes that the callee saved registers are saved if modified by callee.

I therefore think the check !F.hasLocalLinkage() in isSafeForNoCSROpt() should be replaced with !F.isDefinitionExact(), since the former check only covers a subset of the second one.

Otherwise, the saving of the CSRs might be skipped without this being reflected in the call regmask in caller, right?

[vivekvpandya]

In D45308#1083887, @jonpa wrote:

With this patch, any called function which is not "definition exact" keeps the unmodified regmask on the call instruction, which assumes that the callee saved registers are saved if modified by callee.

I therefore think the check !F.hasLocalLinkage() in isSafeForNoCSROpt() should be replaced with !F.isDefinitionExact(), since the former check only covers a subset of the second one.

Otherwise, the saving of the CSRs might be skipped without this being reflected in the call regmask in caller, right?

Yes this needs to be done. Sorry I missed in previous review that isSafeForNoCSROpt() is also used in determineCalleeSaves()

[jonpa]

ping

[kbarton]

In D45308#1071185, @vivekvpandya wrote:

Change looks good to me. But please fix the new test case as suggested by @nemanjai
In this new test do you see improvement in register allocation due to IPRA? If test-case does not show any benefit due to IPRA then it is not suitable test for this patch.
Also I hope this patch is also tested with other architecture which have LIT based test-cases for IPRA. It is very unlikely that such test-case will have function with linkage type which return true for isInterposableLinkage() but if such case is there then this change may break such tests.

This test doesn't show an improvement due to IPRA, because it is testing to ensure IPRA is not run on the bar function because it is marked as weak_odr. Without this fix, IPRA would cause the initialization of r3 and r4 between the calls to bar (CHECKS on line 19 and 20 of the test case) to be removed because bar does not clobber those registers. I have other tests that show the benefit of IPRA that I will include in the patch to enable this on PowerPC.

Yes, this patch has been tested with all of the lit tests, including SystemZ and X86 and there were no failures.

In D45308#1083887, @jonpa wrote:

With this patch, any called function which is not "definition exact" keeps the unmodified regmask on the call instruction, which assumes that the callee saved registers are saved if modified by callee.

I therefore think the check !F.hasLocalLinkage() in isSafeForNoCSROpt() should be replaced with !F.isDefinitionExact(), since the former check only covers a subset of the second one.

Otherwise, the saving of the CSRs might be skipped without this being reflected in the call regmask in caller, right?

llvm/test/CodeGen/PowerPC/ipra-odr.ll

2

I've updated the testcase with -print-regusage. Will upload a new diff soon.

12

Yes, I don't know how that got there. Fixed.

[kbarton]

Added the isDefinitionExact check to isSafeForNoCSROpt method.
Updated the test case to use -print-regusage to check for exact register clobbers computed by IPRA.

[vivekvpandya]

@kbarton can you please validate below tablel?

LinkageType	NoCSROptAllowed before this change	NoCSROptAllowed after this change
WeakAny	not allowed	not allowed
LinkOnce	not allowed	not allowed
Common	not allowed	not allowed
ExternalWeak	not allowed	not allowed
External	not allowed	allowed
Appending	not allowed	allowed (but this linkage does not apply to function)
Internal	allowed	allowed
Private	allowed	allowed
WeakODR	not allowed	not allowed
LinkOnceODR	not allowed	not allowed
Available Externally	not allowed	not allowed

Function with external linkage should not be allowed for noCSR opt. For the attached test-case created for external linkage type I think your change allows noCSROpt foo function as for foo I see following value in debugger.
(lldb) p F.isDefinitionExact()
(bool) $1 = true
(lldb) p F.hasLocalLinkage()
(bool) $2 = false

In Attached files external.ipra.s.1 is with your change and external.ipra.s is TOT.

external.ipra.s.1889 BDownload

external.ipra.ll1 KBDownload

external.ipra.s964 BDownload

[kbarton]

@vivekvpandya You're right - the change will allow noCSROpt to run on functions with external linkage, which is not correct.
I think what we really want here is the isSafeForNoCSROpt to check for the existing conditions (i.e., prior to this patch, when it looks for local linkage) and also whether IPRA was successful for this function. I added the isDefinitionExact() call, to try and replicate the necessary conditions for IPRA, but now I'm wondering if it would be better to query the PRUI and make sure it contains a valid RegMask for the given function. Does this seem reasonable to you?

[vivekvpandya]

In D45308#1145575, @kbarton wrote:

@vivekvpandya You're right - the change will allow noCSROpt to run on functions with external linkage, which is not correct.
I think what we really want here is the isSafeForNoCSROpt to check for the existing conditions (i.e., prior to this patch, when it looks for local linkage) and also whether IPRA was successful for this function. I added the isDefinitionExact() call, to try and replicate the necessary conditions for IPRA, but now I'm wondering if it would be better to query the PRUI and make sure it contains a valid RegMask for the given function. Does this seem reasonable to you?

It should work but I think we can discuss properly if you update this patch as per your expectation.
I assume that you will return early in RegUsageInfoCollector::runOnMachineFunction() if definition is not exact. So we don't have regmask in PRUI for given function.
Now while inserting prologue/epilogue along with isSafeForNoCSROpt() you would like to check if due to above mention condition no regmask is present in PRUI for given function?

Also I realised a potential optimization problem with return early in RegUsageInfoCollector::runOnMachineFunction() for any function without inserting a regmask (even default given as per calling convention for function).
For example there is call sequence like A->B
now as per calling convention for B R1, R2, R3 are not callee saved, but RegUsageInfoCollector detects that it only uses R1, R2 so on TOT version every function will have regmask in PRUI so that RegUsageInforPropogate() can use this data in caller function. In this case A can use the fact that R3 is free to be used. Now with early return due to any condition will make A use default calling convention for B i.e R3 should not be used without spill/restore around call site of B.

[kbarton]

Thanks @vivekvpandya for getting back to me.
I'll work on updating the patch now. I think we're on the same page with the potential problems.
Essentially, there seems to be some assumptions that when EnableIIPRA is set, it will work properly and add additional information that can be used. Now, with the early exit due to isDefinitionExact, that is not necessarily the case. So, I'm trying to put a check in to validate that IPRA was successful before using the information. This way, if we extend the checks in IPRA in the future, we don't need to continually replicate them in other places that rely on IPRA being successful.

Regarding the prologue/epilogue inserter, I don't immediately see any use of the PRUI collected by IPRA. I'll look closer though, as I might have missed something. But, essentially if anyone tries to use the regmask in PRUI, and that mask is NULL, they must assume IPRA was not successful and fallback on the original calling conventions for saving registers across calls.

[MatzeB]

llvm/include/llvm/CodeGen/TargetFrameLowering.h

338

I believe this should rather be
F.hasAvailableExternallyLinkage() || F.isInterposable() || F.hasAddressTaken() || !F.hasFnAttribute(Attribute::NoRecurse).

[MatzeB]

llvm/include/llvm/CodeGen/TargetFrameLowering.h

338

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...)

[MatzeB]

llvm/lib/CodeGen/RegUsageInfoCollector.cpp

104–105

Maybe add a comment that this is a shortcut as the result won't get used?

[kbarton]

llvm/include/llvm/CodeGen/TargetFrameLowering.h

338

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.

llvm/lib/CodeGen/RegUsageInfoCollector.cpp

104–105

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.

[kbarton]

Created a new function (isSafeForIPRA) to consistently check whether IPRA can be performed. This check is used in RegUsageInfoCollector as well as isSafeForNoCSROpt. I also went back to the original checks in isSafeForNoCSROpt to check for LocalLinkage.

[vivekvpandya]

Coge change looks good but I have few questions on lit test cases.

1. in llvm/test/CodeGen/PowerPC/ipra-odr.ll IPRA is not called on bar() then it should follow calling convention so in foo() why registers are getting restored between calls to bar() because without IPRA bar() is expected to save callee-saved-registers as per ABI.
2. llvm/test/CodeGen/X86/ipra-external-linkage.ll same question as above applies. As foo() will save callee-saved-registers as per ABI why around foo() call in bar() it should save/restore callee-saved-register.

llvm/test/CodeGen/X86/ipra-external-linkage.ll

9

For function definition that has external linkage IPRA is applicable but LLVM can't optimize it to have NoCSROpt.
So check is correct but comment needs to be accurate.

[nemanjai]

Is this patch dead? Does @kbarton or perhaps @jonpa have any interest in seeing this through so IPRA can be enabled on PPC/SystemZ?

[kbarton]

In D45308#1449260, @nemanjai wrote:

Is this patch dead? Does @kbarton or perhaps @jonpa have any interest in seeing this through so IPRA can be enabled on PPC/SystemZ?

Thanks for the ping - this had fallen off my radar.
I'd like to get this finalized and committed. I'll post a follow-up patch in the next couple days after I've done my context switch and remembered what the problems were :)

[arsenm]

llvm/include/llvm/CodeGen/TargetFrameLowering.h

353–356

Just return the bool directly?

[vivekvpandya]

Just to note there is related commit https://reviews.llvm.org/rL366557.

llvm/include/llvm/CodeGen/TargetFrameLowering.h

public:
   /// epilogue.
   virtual bool canUseAsEpilogue(const MachineBasicBlock &MBB) const {
     return true;
   }
 
   /// Check if given function is safe for not having callee saved registers.
   /// This is used when interprocedural register allocation is enabled.
   static bool isSafeForNoCSROpt(const Function &F) {
-    if (!F.hasLocalLinkage() || F.hasAddressTaken() ||
+    if (!isSafeForIPRA(F) || !F.hasLocalLinkage() || F.hasAddressTaken() ||
         !F.hasFnAttribute(Attribute::NoRecurse))
       return false;
     // Function should not be optimized as tail call.
     for (const User *U : F.users())
       if (auto CS = ImmutableCallSite(U))
         if (CS.isTailCall())
           return false;
     return true;
   }
 
+  /// Check if given function is safe for IPRA. This check is used both at the
+  /// beginning of IPRA, as well as any additional optimizations that rely on
+  /// IPRA information (e.g., NoCSROpt).
+  static bool isSafeForIPRA(const Function &F) {
+    if (!F.isDefinitionExact())
+      return false;
+
+    return true;
+  }
+
   /// Return initial CFA offset value i.e. the one valid at the beginning of the
   /// function (before any stack operations).
   virtual int getInitialCFAOffset(const MachineFunction &MF) const;
 
   /// Return initial CFA register value i.e. the one valid at the beginning of
   /// the function (before any stack operations).
   virtual unsigned getInitialCFARegister(const MachineFunction &MF) const;
 };
 
 } // End llvm namespace
 
 #endif

llvm/lib/CodeGen/RegUsageInfoCollector.cpp

bool RegUsageInfoCollector::runOnMachineFunction(MachineFunction &MF) {
   // Compute the size of the bit vector to represent all the registers.
   // The bit vector is broken into 32-bit chunks, thus takes the ceil of
   // the number of registers divided by 32 for the size.
   unsigned RegMaskSize = (TRI->getNumRegs() + 31) / 32;
   RegMask.resize(RegMaskSize, 0xFFFFFFFF);
 
   const Function &F = MF.getFunction();
 
+  if (!TargetFrameLowering::isSafeForIPRA(F))
+    return false;
+
   PhysicalRegisterUsageInfo *PRUI = &getAnalysis<PhysicalRegisterUsageInfo>();
 
   PRUI->setTargetMachine(&TM);
 
   LLVM_DEBUG(dbgs() << "Clobbered Registers: ");
 
   BitVector SavedRegs;
   computeCalleeSavedRegs(SavedRegs, MF);

llvm/test/CodeGen/PowerPC/ipra-odr.ll

+; RUN: llc -mtriple=powerpc64le-unknown-linux-gnu -enable-ipra -print-regusage < %s 2>&1 | FileCheck %s
+;
+; Check that IPRA does not run and change register allocation in functions that
+; have callees that could be redefined at link time.
+
+@x = local_unnamed_addr global i64 0, align 4
+
+; Verify that IPRA is not called on bar
+; CHECK-NOT: bar Clobbered Registers:
+define weak_odr void @bar(i64 signext %a, i64 signext %b) local_unnamed_addr {
+entry:
+  %add = add nsw i64 %b, %a
+  store i64 %add, i64* @x, align 4
+  ret void
+}
+
+; Verify that IPRA is called on foo, and that we are restoring registers between calls to bar.
+
+; CHECK:foo Clobbered Registers: $bp $carry $ctr $fp $lr $rm $spefscr $vrsave $xer $zero $bp8 $cr0 $cr1 $cr5 $cr6 $cr7 $ctr8 $f0 $f1 $f2 $f3 $f4 $f5 $f6 $f7 $f8 $f9 $f10 $f11 $f12 $f13 $fp8 $lr8 $qf0 $qf1 $qf2 $qf3 $qf4 $qf5 $qf6 $qf7 $qf8 $qf9 $qf10 $qf11 $qf12 $qf13 $qf14 $qf15 $qf16 $qf17 $qf18 $qf19 $qf20 $qf21 $qf22 $qf23 $qf24 $qf25 $qf26 $qf27 $qf28 $qf29 $qf30 $qf31 $r0 $r1 $r2 $r3 $r4 $r5 $r6 $r7 $r8 $r9 $r10 $r11 $r12 $r13 $s0 $s1 $s2 $s3 $s4 $s5 $s6 $s7 $s8 $s9 $s10 $s11 $s12 $s13 $s14 $s15 $s16 $s17 $s18 $s19 $s20 $s21 $s22 $s23 $s24 $s25 $s26 $s27 $s28 $s29 $s30 $s31 $v0 $v1 $v2 $v3 $v4 $v5 $v6 $v7 $v8 $v9 $v10 $v11 $v12 $v13 $v14 $v15 $v16 $v17 $v18 $v19 $vf0 $vf1 $vf2 $vf3 $vf4 $vf5 $vf6 $vf7 $vf8 $vf9 $vf10 $vf11 $vf12 $vf13 $vf14 $vf15 $vf16 $vf17 $vf18 $vf19 $vsl0 $vsl1 $vsl2 $vsl3 $vsl4 $vsl5 $vsl6 $vsl7 $vsl8 $vsl9 $vsl10 $vsl11 $vsl12 $vsl13 $vsl14 $vsl15 $vsl16 $vsl17 $vsl18 $vsl19 $vsl20 $vsl21 $vsl22 $vsl23 $vsl24 $vsl25 $vsl26 $vsl27 $vsl28 $vsl29 $vsl30 $vsl31 $vsx32 $vsx33 $vsx34 $vsx35 $vsx36 $vsx37 $vsx38 $vsx39 $vsx40 $vsx41 $vsx42 $vsx43 $vsx44 $vsx45 $vsx46 $vsx47 $vsx48 $vsx49 $vsx50 $vsx51 $vsx52 $vsx53 $vsx54 $vsx55 $vsx56 $vsx57 $vsx58 $vsx59 $vsx60 $vsx61 $vsx62 $vsx63 $x0 $x1 $x2 $x3 $x4 $x5 $x6 $x7 $x8 $x9 $x10 $x11 $x12 $x13 $zero8 $cr0eq $cr1eq $cr5eq $cr6eq $cr7eq $cr0gt $cr1gt $cr5gt $cr6gt $cr7gt $cr0lt $cr1lt $cr5lt $cr6lt $cr7lt $cr0un $cr1un $cr5un $cr6un $cr7un
+; CHECK-LABEL: @foo
+; CHECK: bl bar
+; CHECK: nop
+; CHECK: mr 3, {{[0-9]+}}
+; CHECK: mr 4, {{[0-9]+}}
+; CHECK: bl bar
+; CHECK: blr
+define signext i64 @foo(i64 signext %a, i64 signext %b) {
+entry:
+  call void @bar(i64 signext %a, i64 signext %b)
+  call void @bar(i64 signext %a, i64 signext %b)
+
+ret i64 0
+}

llvm/test/CodeGen/X86/ipra-external-linkage.ll

+; RUN: llc < %s | FileCheck %s
+; RUN: llc -enable-ipra < %s | FileCheck %s
+
+target triple = "x86_64--"
+
+define external void @foo() norecurse {
+; When IPRA is not enabled R15 will be saved by foo as it is callee saved reg.
+; When IPRA is enabled we should have the same behaviour as NOIPRA because
+; this is an external function that we cannot optimize with IRPA.
+; CHECK-LABEL: foo:
+; CHECK: pushq %r15
+  call void asm sideeffect "movl  %r14d, %r15d", "~{r15}"()
+  ret void
+}
+
+define void @bar(i32 %X) {
+  call void asm sideeffect "movl  %r12d, $0", "{r15}~{r12}"(i32 %X)
+  ; As R15 is clobbered by foo() when IPRA is enabled value of R15 should be
+  ; saved if register containing orignal value is also getting clobbered
+  ; and reloaded after foo(), here original value is loaded back into R15D after
+  ; call to foo.
+  call void @foo()
+  ; CHECK-LABEL: bar:
+  ; CHECK: movl  %edi, %r[[REG1:[0-9]+]]d
+  ; CHECK: movl  %r[[REG2:[0-9]+]]d, %r[[REG1]]d
+  ; CHECK: callq foo
+  ; CHECK: movl  %r[[REG1]]d, %r[[REG2]]d
+  ; CHECK: popq  %r[[REG1]]
+  ; CHECK: retq
+  call void asm sideeffect "movl  $0, %r12d", "{r15}~{r12}"(i32 %X)
+  ret void
+}