Page MenuHomePhabricator

[Driver] Refactor interaction between -f(no-)?omit-frame-pointer and -m(no-)?omit-leaf-frame-pointer
ClosedPublic

Authored by MaskRay on Jul 6 2019, 11:18 PM.

Details

Summary

Use a tri-state enum to represent shouldUseFramePointer() and
shouldUseLeafFramePointer().

This simplifies the logic and fixes PR9825:

-fno-omit-frame-pointer doesn't imply -mno-omit-leaf-frame-pointer.

and PR24003:

/Oy- /O2 should not omit leaf frame pointer: this matches MSVC x86-32.
(/Oy- is a no-op on MSVC x86-64.)

and:

when CC1 option -mdisable-fp-elim if absent, -momit-leaf-frame-pointer
can also be omitted.

The new behavior matches GCC:

-fomit-frame-pointer wins over -mno-omit-leaf-frame-pointer
-fno-omit-frame-pointer loses out to -momit-leaf-frame-pointer

The behavior makes lots of sense. We have 4 states:

  • 00) leaf retained, non-leaf retained
  • 01) leaf retained, non-leaf omitted (this is invalid)
  • 10) leaf omitted, non-leaf retained (what -momit-leaf-frame-pointer was designed for)
  • 11) leaf omitted, non-leaf omitted

"omit" options taking precedence over "no-omit" options is the only way
to make 3 valid states representable with -f(no-)?omit-frame-pointer and
-m(no-)?omit-leaf-pointer.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jul 6 2019, 11:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2019, 11:18 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ychen added a comment.Jul 8 2019, 9:44 AM

In D56353, I remember @chandlerc thought -f(no-)omit-frame-pointer should win over -m(no-)omit-leaf-frame-pointer. I'm not sure what his thoughts on this now. @chandlerc ?

MaskRay updated this revision to Diff 208615.Jul 9 2019, 1:54 AM
MaskRay edited the summary of this revision. (Show Details)

Implement the GCC behavior:

-fomit-frame-pointer wins over -mno-omit-leaf-frame-pointer
-fno-omit-frame-pointer loses out to -momit-leaf-frame-pointer
MaskRay updated this revision to Diff 208616.Jul 9 2019, 2:07 AM

Simplify and add another test

@ychen The GCC behavior ("omit" wins over "no-omit") makes sense because: in the 4 states:

  • 00) leaf retained, non-leaf retained
  • 01) leaf retained, non-leaf omitted
  • 10) leaf omitted, non-leaf retained
  • 11) leaf omitted, non-leaf omitted

State 10) doesn't make sense. I think -momit-leaf-frame-pointer was designed to omit leaf frame pointer when frame pointer is enabled (-fno-omit-frame-pointer is in action). To make the other 3 states representable, letting "omit" wins over "no-omit" (the current GCC behavior) is the only sensible choice.

ychen added inline comments.Jul 9 2019, 8:34 PM
lib/Driver/ToolChains/Clang.cpp
579 ↗(On Diff #208616)

getFramePointerKind -> decideFramePointerKind / determineFramePointerKind ?

585 ↗(On Diff #208616)

It looks better if frame_pointer is represented using tri-state. Something like this?

It would be great to have comments for conditions that are not obvious such as the overriding rules.

// There are three states for frame_pointer.
enum class FpFlag {true, false, none};
FpFlag FPF = FpFlag::none;
if (Arg *A = Args.getLastArg(options::OPT_fomit_frame_pointer,
                             options::OPT_fno_omit_frame_pointer))
  FPF = A->getOption().matches(options::OPT_fno_omit_frame_pointer)) ?
           FpFlag::true : FpFlag::false;

if (!mustUseNonLeaf && FPF == FpFlag::false)
  return FramePointerKind::None;

if (mustUseNonLeaf || FPF == FpFlag::true || Args.hasArg(options::OPT_pg) ||
    useFramePointerForTargetByDefault(Args, Triple)) {
  if (Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
                   options::OPT_mno_omit_leaf_frame_pointer,
                   Triple.isPS4CPU()))
    return FramePointerKind::NonLeaf;
  return FramePointerKind::All;
}
return FramePointerKind::None;
MaskRay marked an inline comment as done.Jul 10 2019, 1:17 AM
MaskRay added inline comments.
lib/Driver/ToolChains/Clang.cpp
585 ↗(On Diff #208616)

I actually think the current version is clearer.. The local enum class FpFlag {true, false, none}; doesn't improve readability in my opinion.

I can define separate variables for:

  • A && A->getOption().matches(options::OPT_fno_omit_frame_pointer)
  • A && A->getOption().matches(options::OPT_fomit_frame_pointer)

If reviewers think that makes the code easier to read.

ychen added inline comments.Jul 10 2019, 10:19 AM
lib/Driver/ToolChains/Clang.cpp
585 ↗(On Diff #208616)

I think local enum may be optional.

Say

  • Fp = A && A->getOption().matches(options::OPT_fno_omit_frame_pointer)
  • NoFp = A && A->getOption().matches(options::OPT_fomit_frame_pointer)

The !(A && A->getOption().matches(options::OPT_fomit_frame_pointer)) in the current revision could be !A. The implicit logic is NoFp could only be overriden by mustUseNonLeaf.

This block helps to make the implicit logic explicit and simplify the rest of the code.

if (!mustUseNonLeaf && NoFp)
  return FramePointerKind::None;
}
MaskRay updated this revision to Diff 209112.Jul 10 2019, 6:48 PM

Improve readability

MaskRay marked 5 inline comments as done.Jul 10 2019, 6:50 PM
MaskRay added inline comments.
lib/Driver/ToolChains/Clang.cpp
579 ↗(On Diff #208616)

This is a pure function. I think it is fine to use get. This is also consistent with several other get* in this file.

585 ↗(On Diff #208616)

I refactored the code a bit.

I still prefer the current approach to return 3 possible values. Too many return statements just clutter the code I think.

ychen accepted this revision.Jul 11 2019, 9:30 AM

LGTM

This revision is now accepted and ready to land.Jul 11 2019, 9:30 AM
MaskRay updated this revision to Diff 209396.Jul 11 2019, 6:58 PM
MaskRay marked 2 inline comments as done.
MaskRay retitled this revision from [Driver] Consolidate shouldUseFramePointer() and shouldUseLeafFramePointer() to [Driver] Refactor interaction between -f(no-)omit-frame-pointer and -mno-omit-leaf-frame-pointer.
MaskRay edited the summary of this revision. (Show Details)

Update title and description

MaskRay updated this revision to Diff 209398.Jul 11 2019, 7:00 PM
MaskRay retitled this revision from [Driver] Refactor interaction between -f(no-)omit-frame-pointer and -mno-omit-leaf-frame-pointer to [Driver] Refactor interaction between -f(no-)?omit-frame-pointer and -m(no-)?omit-leaf-frame-pointer.
MaskRay edited the summary of this revision. (Show Details)

Fix title

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 7:02 PM