This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer
AbandonedPublic

Authored by ychen on Dec 19 2018, 8:55 PM.

Details

Summary

And make -fomit-frame-pointer imply -momit-leaf-frame-pointer.
This matches GCC's behavior.

llvm.org/PR9825

Diff Detail

Event Timeline

ychen created this revision.Dec 19 2018, 8:55 PM
ychen retitled this revision from [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer and make -fomit-frame-pointer imply -momit-leaf-frame-pointer. This matches GCC's behavior. to [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer.Dec 19 2018, 9:00 PM
ychen edited the summary of this revision. (Show Details)
ychen updated this revision to Diff 179325.EditedDec 21 2018, 11:48 AM
  • update test
chandlerc requested changes to this revision.Dec 23 2018, 1:44 AM
chandlerc added inline comments.
lib/CodeGen/CGCall.cpp
1739

This seems like an unrelated change?

lib/Driver/ToolChains/Clang.cpp
592–595

This doesn't correctly handle "last-flag-wins". Consider the case of -mno-omit-leaf-frame-pointer -fomit-frame-pointer. That should omit the leaf frame pointer, but if I read this correctly the logic here will use a leaf frame pointer.

test/Driver/frame-pointer-elim.c
92

As indicated above, you need a test of the reverse order as well.

This revision now requires changes to proceed.Dec 23 2018, 1:44 AM
ychen updated this revision to Diff 179447.Dec 23 2018, 8:24 AM
ychen marked an inline comment as done.
  • update test
ychen marked 2 inline comments as done.Dec 23 2018, 8:34 AM

Thanks for the review! Comments inline.

lib/CodeGen/CGCall.cpp
1739

The only user of "no-frame-pointer-elim-non-leaf" is TargetOptions::DisableFramePointerElim where "no-frame-pointer-elim-non-leaf" matters only if "no-frame-pointer-elim" is "false". This is to make it less confusing.

lib/Driver/ToolChains/Clang.cpp
592–595

Updated test with this case along with some other cases.

RUN: %clang -### -S -Os -mno-omit-leaf-frame-pointer -fomit-frame-pointer %s 2>&1 | \
RUN: FileCheck --check-prefix=OMIT-ALL5 %s
OMIT-ALL5-NOT: "-mdisable-fp-elim"
OMIT-ALL5-NOT: "-momit-leaf-frame-pointer"

This falls into lib/CodeGen/CGCall.cpp:1733, which causes TargetOptions::DisableFramePointerElim returns false for all frames.

chandlerc requested changes to this revision.Dec 26 2018, 5:47 PM
chandlerc added reviewers: hans, rnk.

The number of negations and such in the CC1 interface here and the attributes makes all of these tests super confusing. Wonder if we should fix that before making changes here.

Also adding folks to comment on the cl.exe driver mode and how it should behave.

lib/CodeGen/CGCall.cpp
1739

Yes, but that's kind of my point. This change is unrelated to the rest of the patch.

I would go ahead and land *just* this change and explain that it doesn't change behavior. Then the actual behavior change can be landed independently.

lib/Driver/ToolChains/Clang.cpp
592–595

Then I don't understand what this change is doing.

This function, when called with arguments -mno-omit-leaf-frame-pointer -fomit-frame-pointer will not hit the code you've added here, and will instead return true. That doesn't seem like a sensible result given the desired change to these flags. If something *else* is causing us to still not use leaf frame pointers, that doesn't make the code here correct, it makes me question how this works at all (and how we are testing it).

test/Driver/cl-options.c
177

Do we want to also change behavior for the CL options? We should discuss this w/ the Windows folks at least....

This revision now requires changes to proceed.Dec 26 2018, 5:47 PM
ychen marked 3 inline comments as done.Dec 26 2018, 7:38 PM
ychen added inline comments.
lib/CodeGen/CGCall.cpp
1739

Got you. Will do.

lib/Driver/ToolChains/Clang.cpp
592–595

I see your point here. The logic is very confusing indeed.

It looks better if
s/shouldUseFramePointer/addFlagDisableFPElim
s/shouldUseLeafFramePointer/addFlagOmitLeafFramePointer

to show that here only decides compiler flag instead of the final code.

I think the correct way to handle these is to replace -mdisable-fp-elim and -momit-leaf-frame-pointer compiler flags with one, say frame-pointer-model = {KeepAll, OmitAll, KeepNonLeaf}, and let the driver decide frame-pointer-model here. The downside is that it affects compiler user unless we bridge deprecating flags on to new flag with some rules.

test/Driver/cl-options.c
177

Sure. It would be great to have them to confirm.

chandlerc added inline comments.Dec 26 2018, 7:49 PM
lib/Driver/ToolChains/Clang.cpp
592–595

Those flags are in the "CC1" interface -- the internal interface between the driver and the compiler internals. That interface has no stability requirements and isn't something we support users leveraging directly.

So I think we could actually change this to be sensible in much the way you are suggesting. Specifically, I would have -frame-pointers={all,non-leaf,none}. And then all of the compatibility and mapping logic in the driver will resolve to a very simple end result.

In code, I think we should follow a similar simplification where we have a single function to determine the total strategy here, rather than the logic being separated into multiple different functions.

rnk added inline comments.Dec 27 2018, 10:34 AM
test/Driver/cl-options.c
177

I'd say this behavior change is desired. I confirmed it's what MSVC does with /Oy-, which is their spelling for this. It also doesn't look like intentional behavior that we implemented to optimize size for 32-bit windows, based on this list of issues here:
https://bugs.llvm.org/showdependencytree.cgi?id=26299&hide_resolved=0
It has the potential to regress code size on i686-windows-msvc, but I don't think 32-bit size really matters at this point. If anything, better stack traces would be preferred.

ychen abandoned this revision.Jul 20 2019, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2019, 4:05 PM