-
Notifications
You must be signed in to change notification settings - Fork 10.5k
IRGen: Always eliminate frame pointers of leaf functions #31921
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IRGen: Always eliminate frame pointers of leaf functions #31921
Conversation
@swift-ci Please test |
@swift-ci Please benchmark |
So we tried this a few times in the past and there were some shocking code size regressions that need to be investigated. |
rdar://59172266 |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibs
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
The code size regressions are due to increased exception handling frame info.
|
Moving code code from the instruction cache onto the disk (eh frame info is cold) at the expense of some disk space is a good trade off IMO. |
Does it make sense to have a knob for this (similar to Rationale: I can see it being useful for freestanding code which needs to unwind itself. |
@swift-ci Please test |
@swift-ci Please benchmark |
Build failed |
Build failed |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibs
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
I have no idea where those perf regressions would come from. I didn't see any regressions on arm64, but feel free to double check... |
@atrick AFAICT clang sets "frame-pointer"="all" (see also my comment on slack) |
I have looked at DataAppendSequence and other than the omitted frame instructions there is no difference in the function where we spend our time. So some kinda of cache effect (alignment or otherwise). |
My questions are Why are we setting the frame pointer explicitly at all when we just want to inherit clang's behavior (at least after John's patch)? And in the case that -no-omit-frame-pointer is used, why do we override it both in |
2627295
to
addf7d8
Compare
@swift-ci Please test |
Build failed |
Build failed |
Of course we can't cherry-pick this version to 5.3 since John's change won't be there. Going back to the previous change. |
addf7d8
to
109813f
Compare
@swift-ci Please test |
Build failed |
Build failed |
@swift-ci Please test |
This code makes it seem as if we're still telling clang to emit all frame pointers
I don't know if you should remove the code completely, or add a check for DisableFPElim and set it to FramePointerKind::NonLeaf |
This only affects C functions that we import (i.e inline foo()) until we remove the call to |
db0135f
to
35dceab
Compare
@swift-ci Please test |
Build failed |
Build failed |
@swift-ci Please test |
Build failed |
Build failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this patch will do the right thing. Approved for 5.3!
For after John's patch: the design of the flags is still unacceptably bad. There should only be one place (in the driver) where we interpret the sense of the options: disable/enable/emit/omit whatever! There should be only one place where we check the IRGen flags--why is it different for imported functions? Those flags should first indicate whether the Swift driver has overridden clang's frame-pointer default so it's obvious to someone reading the code that the IRGen options are normally irrelevant. There should probably be two IRGen options:
- overrideLLVMFramePointerAttribute = T/F (clearly for experimentation only)
- framePointerAttributeOverride = none/nonleaf/all (clearly only only when the swift driver sets a flag)
Or make flag #2 optional if that's possible.
Finally, some Swift functions will override both LLVM and Swift driver flags to suppress frame pointers. And this should all be commented.
@swift-ci Please test |
Build failed |
Build failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isArch64() check LGTM
@swift-ci Please test |
Build failed |
Build failed |
This seems to have caused a regression on most of the bots. I think that the test needs to be adjusted. Reverting until this can be looked at. |
Which bot? |
Failures in framepointer_arm64.sil should be addressed by #31987. |
rdar://20933449