Launchpad has imported 15 comments from the remote bug at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40887.
If you reply to an imported comment from within Launchpad, your comment will be sent to the remote bug automatically. Read more about Launchpad's inter-bugtracker facilities at https://documentation.ubuntu.com/launchpad/user/reference/bugs/multi-project-bugs/about-multi-project-bugs/#bugs-in-external-trackers. ------------------------------------------------------------------------ On 2009-07-28T05:10:57+00:00 Lessen42+gcc wrote: Consider the following code: int (*indirect_func)(); int indirect_call() { return indirect_func(); } gcc 4.4.0 generates the following with -O2 -mcpu=cortex-a8 -S: indirect_call: @ args = 0, pretend = 0, frame = 0 @ frame_needed = 0, uses_anonymous_args = 0 movw r3, #:lower16:indirect_func stmfd sp!, {r4, lr} movt r3, #:upper16:indirect_func mov lr, pc ldr pc, [r3, #0] ldmfd sp!, {r4, pc} The problem is that the instruction "ldr pc, [r3, #0]" is not considered a function call by the Cortex-A8's branch predictor, as noted in DDI0344J section 5.2.1, Return stack predictions. Thus, the return from the called function is mispredicted resulting in a penalty of 13 cycles compared to a direct call. Rather than doing mov lr, pc ldr pc, [r3] it should instead use the blx instruction as so: ldr lr, [r3] blx lr which is considered a function call by the branch predictor, and has an overhead of only one cycle compared to a direct call. gcc -v: Using built-in specs. Target: arm-none-linux-gnueabi Configured with: ../gcc-4.4.0/configure --target=arm-none-linux-gnueabi --prefix=/usr/local/arm --enable-threads --with-sysroot=/usr/local/arm/arm-none-linux-gnueabi/libc Thread model: posix gcc version 4.4.0 (GCC) Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/0 ------------------------------------------------------------------------ On 2009-07-28T05:14:34+00:00 Lessen42+gcc wrote: Created attachment 18261 Use blx for indirect function calls on armv5+ This fixes the test case and the obvious cases of this I found in x264; there may be more instances of not calling/returning from a function that doesn't match Cortex-A8 and A9's branch predictors (and maybe more) Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/1 ------------------------------------------------------------------------ On 2009-07-28T08:29:41+00:00 Ramana-gcc wrote: (In reply to comment #0) > Consider the following code: > > int (*indirect_func)(); > > int indirect_call() > { > return indirect_func(); > } > > gcc 4.4.0 generates the following with -O2 -mcpu=cortex-a8 -S: > > indirect_call: > @ args = 0, pretend = 0, frame = 0 > @ frame_needed = 0, uses_anonymous_args = 0 > movw r3, #:lower16:indirect_func > stmfd sp!, {r4, lr} > movt r3, #:upper16:indirect_func > mov lr, pc > ldr pc, [r3, #0] > ldmfd sp!, {r4, pc} > > The problem is that the instruction "ldr pc, [r3, #0]" is not considered a > function call by the Cortex-A8's branch predictor, as noted in DDI0344J > section > 5.2.1, Return stack predictions. Thus, the return from the called function is > mispredicted resulting in a penalty of 13 cycles compared to a direct call > > Rather than doing > mov lr, pc > ldr pc, [r3] > it should instead use the blx instruction as so: > ldr lr, [r3] > blx lr > which is considered a function call by the branch predictor, and has an > overhead of only one cycle compared to a direct call. The point made is correct but there is something you've missed in your patch ! loading lr with the address of the function you want to call, destroys the return address ,- so your code is never going to return ! Instead you want - ldr r3,[r3] blx r3 Or better still bx r3 but that is PR19599 :) > > gcc -v: > Using built-in specs. > Target: arm-none-linux-gnueabi > Configured with: ../gcc-4.4.0/configure --target=arm-none-linux-gnueabi > --prefix=/usr/local/arm --enable-threads > --with-sysroot=/usr/local/arm/arm-none-linux-gnueabi/libc > Thread model: posix > gcc version 4.4.0 (GCC) > Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/2 ------------------------------------------------------------------------ On 2009-07-28T08:45:25+00:00 Lessen42+gcc wrote: (In reply to comment #2) > The point made is correct but there is something you've missed in your patch ! > loading lr with the address of the function you want to call, destroys the > return address ,- so your code is never going to return ! > > Instead you want - > > ldr r3,[r3] > blx r3 > > Or better still bx r3 but that is PR19599 :) blx sets the link register to the correct return address as a part of the instruction, and the return address of the calling function has to already have been saved before this point or the mov lr, pc would destroy it already. Though perhaps the code should just always use ip with armv5+ like on the other side of the if() since that's callee saved and the tail- optimized "bx lr" would be suboptimal, since that's considered a function return. Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/3 ------------------------------------------------------------------------ On 2009-07-28T12:09:01+00:00 Ramana-gcc wrote: (In reply to comment #3) > (In reply to comment #2) > > The point made is correct but there is something you've missed in your > > patch ! > > loading lr with the address of the function you want to call, destroys the > > return address ,- so your code is never going to return ! > > > > Instead you want - > > > > ldr r3,[r3] > > blx r3 > > > > Or better still bx r3 but that is PR19599 :) > > blx sets the link register to the correct return address as a part of the > instruction, and the return address of the calling function has to already > have > been saved before this point or the mov lr, pc would destroy it already. Oops yes, you are right - I must have been asleep ! . I would rather split the load out as a separate insn and allow it to be scheduled separately. Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/4 ------------------------------------------------------------------------ On 2009-07-28T14:24:07+00:00 Mans Rullgard wrote: Just to be clear, this bug report is about *all* calls through function pointers. PR19599 only mentions a failed tail-call optimisation. That the example in this bug would benefit from this optimisation is secondary. I agree about splitting the operations to allow better scheduling. Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/5 ------------------------------------------------------------------------ On 2009-12-21T08:27:12+00:00 Siarhei Siamashka wrote: Created attachment 19356 return-address-prediction-bench.c This looks like a really serious performance issue. Not just indirect call alone is penalized, but the whole return address prediction stack is busted, causing return address mispredictions for all the nested calls. The attached test program demonstrates it. $ time ./return-address-prediction-bench 1 Indirect call for the topmost function real 0m0.793s user 0m0.789s sys 0m0.000s $ time ./return-address-prediction-bench Indirect call for the leaf function real 0m1.797s user 0m1.789s sys 0m0.008s gcc 4.4.2, "-O2 -mcpu=cortex-a8" Change of function pointer type "void (*f)()" -> "void (* volatile f)()" can be also used to "workaround" the problem. In this case execution times for both variants of test are approximately the same. Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/6 ------------------------------------------------------------------------ On 2009-12-21T08:53:40+00:00 Siarhei Siamashka wrote: (In reply to comment #4) > I would rather split the load out as a separate insn and allow it to be > scheduled separately. A question just to clarify the status of this issue. Are you waiting for David (or anybody else) to provide an updated patch with such split load? Are there no other options available besides either a perfect fix or no fix at all? Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/7 ------------------------------------------------------------------------ On 2009-12-23T09:00:58+00:00 Ramana-gcc wrote: Patch submitted here. http://gcc.gnu.org/ml/gcc- patches/2009-12/msg01060.html Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/8 ------------------------------------------------------------------------ On 2009-12-24T10:46:24+00:00 Ramana-gcc wrote: Subject: Bug 40887 Author: ramana Date: Thu Dec 24 10:46:00 2009 New Revision: 155453 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=155453 Log: Fix PR target/40887 2009-12-24 Julian Brown <[email protected]> Ramana Radhakrishnan <[email protected]> PR target/40887 * config/arm/arm.c (output_call_mem): Remove armv5 support. * config/arm/arm.md (*call_mem): Disable for armv5. Add note. (*call_value_mem): Likewise. PR target/40887 * gcc.target/gcc.arm/pr40887.c: New test. Added: trunk/gcc/testsuite/gcc.target/arm/pr40887.c Modified: trunk/gcc/ChangeLog trunk/gcc/config/arm/arm.c trunk/gcc/config/arm/arm.md trunk/gcc/testsuite/ChangeLog Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/9 ------------------------------------------------------------------------ On 2010-01-07T14:42:10+00:00 Drow-l wrote: Ramana, is this fixed or are you planning on applying it to more branches? Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/10 ------------------------------------------------------------------------ On 2010-01-07T15:03:42+00:00 Ramana-gcc wrote: (In reply to comment #10) > Ramana, is this fixed or are you planning on applying it to more branches? > This is fixed on trunk. I plan to put this on 4.4 branch but it won't be before the end of the week that I can get around to testing this. cheers Ramana Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/11 ------------------------------------------------------------------------ On 2010-02-18T13:13:16+00:00 Ramana-gcc wrote: Subject: Bug 40887 Author: ramana Date: Thu Feb 18 13:13:03 2010 New Revision: 156862 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=156862 Log: PR target/40887 Backport from trunk. 2009-12-24 Julian Brown <[email protected]> Ramana Radhakrishnan <[email protected]> * config/arm/arm.c (output_call_mem): Remove armv5 support. * config/arm/arm.md (*call_mem): Disable for armv5. Add note. (*call_value_mem): Likewise. PR target/40887 Backport from trunk. 2009-12-24 Julian Brown <[email protected]> Ramana Radhakrishnan <[email protected]> * gcc.target/arm/pr40887.c: New test. Added: branches/gcc-4_4-branch/gcc/testsuite/gcc.target/arm/pr40887.c - copied unchanged from r155453, trunk/gcc/testsuite/gcc.target/arm/pr40887.c Modified: branches/gcc-4_4-branch/gcc/ChangeLog branches/gcc-4_4-branch/gcc/config/arm/arm.c branches/gcc-4_4-branch/gcc/config/arm/arm.md branches/gcc-4_4-branch/gcc/testsuite/ChangeLog Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/12 ------------------------------------------------------------------------ On 2010-02-18T13:34:09+00:00 Ramana-gcc wrote: Fixed. Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/13 ------------------------------------------------------------------------ On 2010-06-07T20:59:54+00:00 Grosser-5 wrote: Subject: Bug 40887 Author: grosser Date: Mon Jun 7 20:59:33 2010 New Revision: 160403 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=160403 Log: Merge some commits missed during a merge from mainline in Dez 2009. * README: Mention changes to Makefile.in and functions.texi. * gather-docs: Mention 'make stamp-functions' in the header. svn+ssh://gcc.gnu.org/svn/gcc/trunk@154545 * functions.texi: Rebuild. svn+ssh://gcc.gnu.org/svn/gcc/trunk@154546 Fix PR target/40887 2009-12-24 Julian Brown <[email protected]> Ramana Radhakrishnan <[email protected]> PR target/40887 * config/arm/arm.c (output_call_mem): Remove armv5 support. * config/arm/arm.md (*call_mem): Disable for armv5. Add note. (*call_value_mem): Likewise. PR target/40887 * gcc.target/gcc.arm/pr40887.c: New test. svn+ssh://gcc.gnu.org/svn/gcc/trunk@155453 Fix PR target/42093 2009-12-23 Ramana Radhakrishnan <[email protected]> PR target/42093 * config/arm/arm.h (CASE_VECTOR_PC_RELATIVE): Fix macro usage to TARGET_THUMB1. (CASE_VECTOR_SHORTEN_MODE): Allow signed offsets only for TARGET_THUMB1. 2009-12-23 Ramana Radhakrishnan <[email protected]> PR target/42093 * gcc.target/arm/pr42093.c: New test. svn+ssh://gcc.gnu.org/svn/gcc/trunk@155428 PR debug/42454 * dwarf2out.c (add_ranges_by_labels_to_AT_range_list): New function. (dwarf2out_finish): Call add_ranges_by_labels_to_AT_range_list. * gcc.dg/debug/dwarf2/aranges-fnsec-1.c: Add check for .debug_ranges. svn+ssh://gcc.gnu.org/svn/gcc/trunk@155429 Added: branches/graphite/gcc/testsuite/gcc.target/arm/pr40887.c branches/graphite/gcc/testsuite/gcc.target/arm/pr42093.c Modified: branches/graphite/gcc/config/arm/arm.c branches/graphite/gcc/config/arm/arm.h branches/graphite/gcc/config/arm/arm.md branches/graphite/gcc/dwarf2out.c branches/graphite/gcc/testsuite/gcc.dg/debug/dwarf2/aranges-fnsec-1.c branches/graphite/libiberty/README branches/graphite/libiberty/functions.texi branches/graphite/libiberty/gather-docs Reply at: https://bugs.launchpad.net/ubuntu/+source/gcc-4.4/+bug/525204/comments/21 -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/525204 Title: [PR40887] Backport indirection function call opt To manage notifications about this bug go to: https://bugs.launchpad.net/gcc/+bug/525204/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
