This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Assign stack locations to increase load/store pairing.
AbandonedPublic

Authored by gberry on Aug 10 2016, 1:02 PM.

Details

Summary

Use TargetFrameLowering::orderFrameObjects hook to change allocation
order of stack frame locations to increase the number of load/store pair
instructions generated.

Event Timeline

gberry updated this revision to Diff 67581.Aug 10 2016, 1:02 PM
gberry retitled this revision from to [AArch64] Assign stack locations to increase load/store pairing..
gberry updated this object.
gberry added reviewers: mcrosier, t.p.northover.
gberry added a subscriber: llvm-commits.
mcrosier edited edge metadata.Aug 16 2016, 1:07 PM

The code looks reasonable and it appears to do what's advertised.

Did you measure how many more instructions are paired? I assume this is mostly a code size reduction optimization. Did you run into any issues with compile-time?

test/CodeGen/AArch64/stack-layout-pairing.ll
39

I don't understand this comment about optimizing for size..

Adding James as he seems interested in code size reduction optimizations as of late.

Here are some relevant stat diffs from compiling the llvm test-suite:

   27 (+0.10 %) aarch64-ldst-opt - Number of load/store from unscaled generated     
 2276 (+2.18 %) aarch64-ldst-opt - Number of load/store pair instructions generated 
    8 (+1.74 %) aarch64-ldst-opt - Number of narrow zero stores promoted            
    5 (+0.02 %) branchfolding - Number of block tails merged                        
    2 (+0.00 %) branchfolding - Number of branches optimized                        
    5 (+0.01 %) branchfolding - Number of dead blocks removed                       
-2297 (-0.08 %) mccodeemitter - Number of MC instructions emitted.                  
-2928 (-0.02 %) pei - Number of bytes used for stack in all functions

I'm still collecting compile time data.

test/CodeGen/AArch64/stack-layout-pairing.ll
39

Fixed. That was left over from a previous version of the change.

My compile-time testing showed no significant differences when compiling the llvm test-suite benchmarks at -O1

jmolloy edited edge metadata.Sep 12 2016, 8:59 AM

Hi Geoff,

Sorry, I'll take a look at this tomorrow!

James

This generally looks good to me, although I haven't had a super in depth look. Given the code complexity it'd be nice to have more than 2 testcases though.

James

lib/Target/AArch64/AArch64FrameLowering.cpp
1230

referenced*

1237

Can't this just be "const"?

This generally looks good to me, although I haven't had a super in depth look. Given the code complexity it'd be nice to have more than 2 testcases though.

James

gberry updated this revision to Diff 75215.Oct 19 2016, 2:04 PM
gberry edited edge metadata.

Update to address James' comments.

James,

Thanks for taking a look. I can write more tests, but I don't really know that I can increase the code coverage in doing so, since the code doesn't really have many different cases to handle. Do you have any thoughts on what kind of additional testing should be done?

gberry updated this revision to Diff 85031.Jan 19 2017, 2:21 PM

Update to add more tests cases and fix issue with run-to-run stability of output.

Ping? I've added some more test cases (and a couple more reviewers in case any of you would care to take a look).

Can you please add vector types to the test?

gberry updated this revision to Diff 85047.Jan 19 2017, 3:13 PM

Update to add vector load pair test.

MatzeB resigned from this revision.Aug 15 2017, 11:06 AM
gberry abandoned this revision.Dec 22 2017, 12:41 PM

I'll resubmit this for review if I ever get back around to it