IRGen: Default to clang's frame pointer elimination settings #31986
Conversation
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
aschwaighofer
force-pushed
the
irgen_inherit_clangs_fp_elim
branch
from
cf280e8
to
e19ce39
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
There was a problem hiding this comment.
This covers the important issues
- inherit Clang's default
- Make IRGen readable.
I don't know for sure what we should be doing for the driver flags, or whether they should be overriding IRGen's decisions. Whatever we do I just want to know that it was intentional.
It seems like the only thing that should override IRGen's decision is if we have framePointer==All. Changing leaf FP's should not override them right?
If only the leaf flag is used, we either need to assume a clang default or ask clang for the default. Either way, I think that should be clearly commented because it's not obvious that changing leaf-fp will override non-leaf fp behavior.
It's very strange that -no-omit-frame-pointer and -no-omit-frame-pointer-leaf are actually the same option.
I think we have the option of either copying clang's current logic, or handling the full matrix of possibilities. This case is especially confusing
-omit-frame-pointer + -no-omit-frame-pointer-leaf
lib/Frontend/CompilerInvocation.cpp
Outdated
| bool omitLeafFP = Args.hasArg(options::OPT_omit_frame_pointer_leaf); | ||
| bool requiresAllFP = Args.hasArg(options::OPT_no_omit_frame_pointer); | ||
|
|
||
| // If the user explicitly requests -omit-frame-pointer do we emit no |
lib/Frontend/CompilerInvocation.cpp
Outdated
| /// Determine which frame pointer setting to use. Per default if no flag is set | ||
| /// we will use clang's default setting. This case is denoted by returning None. | ||
| static Optional<clang::CodeGenOptions::FramePointerKind> | ||
| determineFramePointerUse(ArgList &Args) { |
There was a problem hiding this comment.
Does this follow the same logic in the clang driver? I didn't check.
lib/Frontend/CompilerInvocation.cpp
Outdated
| if (omitAllFP) | ||
| return clang::CodeGenOptions::FramePointerKind::None; | ||
|
|
||
| // If the user explicitly requests -omit-frame-pointer do we emit no |
lib/Frontend/CompilerInvocation.cpp
Outdated
| if (omitLeafFP) | ||
| return clang::CodeGenOptions::FramePointerKind::NonLeaf; | ||
|
|
||
| // Otherwise, if the users requires leaf frame pointer we emit all frame |
lib/Frontend/CompilerInvocation.cpp
Outdated
| // If the user explicitly requests -omit-frame-pointer do we emit no | ||
| // frame pointers. | ||
| if (omitAllFP) | ||
| return clang::CodeGenOptions::FramePointerKind::None; |
There was a problem hiding this comment.
What if no_omit_frame_pointer_leaf is set? then shouldn't it be NonLeaf?
There was a problem hiding this comment.
omit_frame_pointer + no_omit_frame_pointer_leaf means omitting the frame pointer in all frames except for leaf frames.
frame-pointer="non-leaf" means to have a frame pointer only in non-leaf functions.
LLVM has no way of expressing this combination. So omit-frame-pointer overriding everything else makes sense.
This is also what clang does, it calls this combination invalid: https://github.com/apple/llvm-project/blob/apple/master/clang/lib/Driver/ToolChains/Clang.cpp#L594
lib/Frontend/CompilerInvocation.cpp
Outdated
| // If the user explicitly requests -omit-frame-pointer do we emit no | ||
| // frame pointers. | ||
| if (omitLeafFP) | ||
| return clang::CodeGenOptions::FramePointerKind::NonLeaf; |
There was a problem hiding this comment.
Assumes a default of no_omit_frame_pointer. I don't know if that's intended.
There was a problem hiding this comment.
Yes this is intended.
I don't know what case you are concerned about?
In this branch we can only have:
-fomit-frame-pointer-leaf in which case frame-pointer="non-leaf' is the right thing
-fno-omit-frame-pointer -fomit-frame-pointer-leaf in which case frame-pointer="non-leaf' is also right thing
We can't have
-fomit-frame-pointer because that is already checked at this point.
If clang defaults to none then we will override this here.
lib/Frontend/CompilerInvocation.cpp
Outdated
| // Otherwise, if the users requires leaf frame pointer we emit all frame | ||
| // pointers. | ||
| if (requiresLeafFramePointers) | ||
| return clang::CodeGenOptions::FramePointerKind::All; |
There was a problem hiding this comment.
Assumes a default of no_omit_frame_pointer. I don't know if that's intended.
What if we also have requiresFP?
There was a problem hiding this comment.
This is intended.
In the branch we can only have:
-fno-omit-frame-pointer-leaf in which case frame-pointer="all" makes sense
-fno-omit-frame-pointer-leaf -fno-omit-frame-pointer in which case "all" also makes sense.
It is a reasonable consequence from the fact that we can't represent frame-pointer = "only-leaf". default=all |
You are concerned about the case with a clang default of frame-pointer=none and -omit-frame-pointer-leaf we get frame-pointer=non-leaf instead of staying in none. I see. I find it more confusing that I have to know what the default is to gauge the outcome but I see your point. |
|
We can get around all of this by just letting clang options set whatever is wanted and don't have to add swift options for this i.e it is possible to just specify -Xcc -fomit-frame-pointer. |
aschwaighofer
force-pushed
the
irgen_inherit_clangs_fp_elim
branch
from
e19ce39
to
d0184de
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
Clang provides options to override that default value. These options are accessible via the -Xcc flag. Some Swift functions explicitly disable the frame pointer. The clang options will not override those.
aschwaighofer
force-pushed
the
irgen_inherit_clangs_fp_elim
branch
from
d0184de
to
20f4ef9
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test linux |
|
Build failed |
|
@swift-ci Please smoke test linux |
|
@swift-ci Please test linux |
|
Build failed |
|
@swift-ci Please clean test linux |
|
This seems to have regressed the windows builders: https://ci-external.swift.org/job/oss-swift-windows-x86_64-vs2019/1563/consoleText |
|
@compnerd Swift now behaves the same as clang does wrt to frame pointers. It is possible that that means on windows it behaves slightly differently to what the test expects. Let me see. |
…S2017. In order to generate stack traces when exceptions happen, it seems that VS2017 needs of frame pointers in some of the functions. This recovers a small override that was present up to the introduction of apple#31986 but only for VS2017. Without this change crash-in-user-code.swift test fails (because the output doesn't include the stack trace as provided by LLVM exception stack trace capturing), but the test passes with it. In order to not change VS2019 (or other compilers), which succeed at the test already, the fix is gated to only compile in VS2017.
Clang provides options to override that default value.
These options are accessible via the -Xcc flag.
Some Swift functions explicitly disable the frame pointer.
The clang options will not override those.