Details
- Reviewers
chandlerc rnk t.p.northover MaskRay - Commits
- rGff22ec3d7004: [Clang] Replace cc1 options '-mdisable-fp-elim' and '-momit-leaf-frame-pointer'…
rL366645: [Clang] Replace cc1 options '-mdisable-fp-elim' and '-momit-leaf-frame-pointer'
rC366645: [Clang] Replace cc1 options '-mdisable-fp-elim' and '-momit-leaf-frame-pointer'
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 35050 Build 35049: arc lint + arc unit
Event Timeline
Wow, thanks for the cleanups. This is much easier to read as a consequence. And sorry it took a while to get you another round of review. Comments below.
include/clang/Driver/CC1Options.td | ||
---|---|---|
270 ↗ | (On Diff #180368) | Should say more Specify which frame pointers to retain (all, non-leaf, none). |
lib/Driver/ToolChains/Clang.cpp | ||
574 ↗ | (On Diff #180368) | Keep LLVM coding convention naming pattern please. |
576–579 ↗ | (On Diff #180368) | This still doesn't make sense to me... If the user specifies -fomit-frame-point or -fno-omit-frame-pointer *after* -momit-leaf-frame-pointer or -mno-omit-leaf-frame-pointer, then that last flag should win... I think you need to first get the "base" FP state by checking the main two flags. Then you need to get the "last" FP state by checking *all four flags*. When the last flag is a leaf flag, then the state is determined by the base + the last. When the last flag is not one of the leaf flags, then the last flag fully specifies the result. I think you can also process these variables into something simpler to test below, essentially handling all the matching logic in one place. |
lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
576–579 ↗ | (On Diff #180368) | I see the point here. I didn't do it since it deviates from GCC/intel compiler behavior. Could you please confirm we really want that? One example from here is : -momit-leaf-frame-pointer -fno-omit-frame-pointer For GCC/intel, this is frame-pointer=non-leaf, for us, it will be frame-pointer=all |
clang/include/clang/Basic/CodeGenOptions.h | ||
---|---|---|
120 | We can probably use enum class FramePointerKind { None, NonLeaf, All }; here.. (enum class ClassABI is an example in this file) | |
clang/lib/CodeGen/CGCall.cpp | ||
1734 | llvm_unreachable can be deleted if a switch statement is used. | |
clang/lib/Driver/ToolChains/Clang.cpp | ||
49 | This is probably not necessary. The enum is only used in 2 places below. | |
3972 | llvm_unreachable("unknown FramePointerKind"); can be deleted if a switch statement is used. | |
clang/lib/Frontend/CompilerInvocation.cpp | ||
845 | I guess this can be deleted now. -pg + FramePointerKind::None is rejected by the driver. if (Arg *A = Args.getLastArg(options::OPT_pg)) if (FPKeepKind == FramePointerKind::None) D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer" << A->getAsString(Args); |
clang/lib/Frontend/CompilerInvocation.cpp | ||
---|---|---|
845 | This is still needed for clang/test/CodeGen/x86_64-profiling-keep-fp.c to pass. We could have another patch to address this and update the test. |
- Make FramePointerKind enum class.
- Replace two if-else-if blocks to switch statements.
We can probably use enum class FramePointerKind { None, NonLeaf, All }; here.. (enum class ClassABI is an example in this file)