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

[ychen]

Summary

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

llvm.org/PR9825

[ychen]

• update test

[chandlerc]

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.

[ychen]

• update test

[ychen]

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]

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....

[ychen]

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]

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]

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]

https://reviews.llvm.org/D56351
https://reviews.llvm.org/D56353

lib/CodeGen/CGCall.cpp

if (AttrOnCallSite) {
     // Attributes that should go on the function, but not the call site.
     if (!CodeGenOpts.DisableFPElim) {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "false");
     } else if (CodeGenOpts.OmitLeafFramePointer) {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "false");
       FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
     } else {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
-      FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
     }
 
     FuncAttrs.addAttribute("less-precise-fpmad",
                            llvm::toStringRef(CodeGenOpts.LessPreciseFPMAD));
 
     if (CodeGenOpts.NullPointerIsValid)
       FuncAttrs.addAttribute("null-pointer-is-valid", "true");
     if (!CodeGenOpts.FPDenormalMode.empty())

lib/Driver/ToolChains/Clang.cpp

static codegenoptions::DebugInfoKind DebugLevelToInfoKind(const Arg &A) {
   if (A.getOption().matches(options::OPT_gline_tables_only) ||
       A.getOption().matches(options::OPT_ggdb1))
     return codegenoptions::DebugLineTablesOnly;
   if (A.getOption().matches(options::OPT_gline_directives_only))
     return codegenoptions::DebugDirectivesOnly;
   return codegenoptions::LimitedDebugInfo;
 }
 
-static bool mustUseNonLeafFramePointerForTarget(const llvm::Triple &Triple) {
+static bool mustUseFramePointerForTarget(const llvm::Triple &Triple) {
   switch (Triple.getArch()){
   default:
     return false;
   case llvm::Triple::arm:
   case llvm::Triple::thumb:
     // ARM Darwin targets require a frame pointer to be always present to aid
     // offline debugging via backtraces.
     return Triple.isOSDarwin();
static bool useFramePointerForTargetByDefault(const ArgList &Args,
   return true;
 }
 
 static bool shouldUseFramePointer(const ArgList &Args,
                                   const llvm::Triple &Triple) {
   if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
                                options::OPT_fomit_frame_pointer))
     return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
-           mustUseNonLeafFramePointerForTarget(Triple);
+           mustUseFramePointerForTarget(Triple);
 
   if (Args.hasArg(options::OPT_pg))
     return true;
 
   return useFramePointerForTargetByDefault(Args, Triple);
 }
 
 static bool shouldUseLeafFramePointer(const ArgList &Args,
                                       const llvm::Triple &Triple) {
   if (Arg *A = Args.getLastArg(options::OPT_mno_omit_leaf_frame_pointer,
                                options::OPT_momit_leaf_frame_pointer))
     return A->getOption().matches(options::OPT_mno_omit_leaf_frame_pointer);
 
+  if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+                               options::OPT_fomit_frame_pointer))
+    return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
+           mustUseFramePointerForTarget(Triple);
+
   if (Args.hasArg(options::OPT_pg))
     return true;
 
   if (Triple.isPS4CPU())
     return false;
 
   return useFramePointerForTargetByDefault(Args, Triple);
 }

test/Driver/cl-options.c

 // RUN: %clang_cl /Ox --target=i686-pc-windows-msvc -### -- %s 2>&1 | FileCheck -check-prefix=Ox %s
 // RUN: %clang_cl /Ox --target=x86_64-pc-windows-msvc -### -- %s 2>&1 | FileCheck -check-prefix=Ox %s
 // Ox-NOT: -mdisable-fp-elim
 // Ox: -momit-leaf-frame-pointer
 // Ox: -O2
 
 // RUN: %clang_cl --target=i686-pc-win32 /O2sy- -### -- %s 2>&1 | FileCheck -check-prefix=PR24003 %s
 // PR24003: -mdisable-fp-elim
-// PR24003: -momit-leaf-frame-pointer
 // PR24003: -Os
 
 // RUN: %clang_cl --target=i686-pc-win32 -Werror /Oy- /O2 -### -- %s 2>&1 | FileCheck -check-prefix=Oy_2 %s
-// Oy_2: -momit-leaf-frame-pointer
+// Oy_2: -mdisable-fp-elim
 // Oy_2: -O2
 
 // RUN: %clang_cl --target=i686-pc-win32 -Werror /O2 /O2 -### -- %s 2>&1 | FileCheck -check-prefix=O2O2 %s
 // O2O2: "-O2"
 
 // RUN: %clang_cl /Zs -Werror /Oy -- %s 2>&1
 
 // RUN: %clang_cl --target=i686-pc-win32 -Werror /Oy- -### -- %s 2>&1 | FileCheck -check-prefix=Oy_ %s

test/Driver/frame-pointer-elim.c

 
 // On the PS4, we default to omitting the frame pointer on leaf functions
 // (OMIT_LEAF check line is above)
 // RUN: %clang -### -target x86_64-scei-ps4 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=OMIT_LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S -O2 %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=OMIT_LEAF %s
 
+// RUN: %clang -### -S -Os -fno-omit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=NO-OMIT-ALL %s
+// NO-OMIT-ALL: "-mdisable-fp-elim"
+// NO-OMIT-ALL-NOT: "-momit-leaf-frame-pointer"
+
+// RUN: %clang -### -S -Os -fomit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=OMIT-ALL %s
+// OMIT-ALL-NOT: "-mdisable-fp-elim"
+// OMIT-ALL: "-momit-leaf-frame-pointer"
+
+// RUN: %clang -### -S -Os -fno-omit-frame-pointer -momit-leaf-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=NO-OMIT-NONLEAF %s
+// NO-OMIT-NONLEAF: "-mdisable-fp-elim"
+// NO-OMIT-NONLEAF: "-momit-leaf-frame-pointer"
+
+// RUN: %clang -### -S -Os -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=NO-OMIT-ALL2 %s
+// NO-OMIT-ALL2: "-mdisable-fp-elim"
+// NO-OMIT-ALL2-NOT: "-momit-leaf-frame-pointer"
+
+// RUN: %clang -### -S -Os -fomit-frame-pointer -momit-leaf-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=OMIT-ALL2 %s
+// OMIT-ALL2-NOT: "-mdisable-fp-elim"
+// OMIT-ALL2: "-momit-leaf-frame-pointer"
+
+// RUN: %clang -### -S -Os -fomit-frame-pointer -mno-omit-leaf-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=OMIT-ALL3 %s
+// OMIT-ALL3-NOT: "-mdisable-fp-elim"
+// OMIT-ALL3-NOT: "-momit-leaf-frame-pointer"
+
+// RUN: %clang -### -S -Os -momit-leaf-frame-pointer -fomit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=OMIT-ALL4 %s
+// OMIT-ALL4-NOT: "-mdisable-fp-elim"
+// OMIT-ALL4: "-momit-leaf-frame-pointer"
+
+// RUN: %clang -### -S -Os -momit-leaf-frame-pointer -fno-omit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=NO-OMIT-NONLEAF2 %s
+// NO-OMIT-NONLEAF2: "-mdisable-fp-elim"
+// NO-OMIT-NONLEAF2: "-momit-leaf-frame-pointer"
+
+// 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"
+
+// RUN: %clang -### -S -Os -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=NO-OMIT-ALL3 %s
+// NO-OMIT-ALL3: "-mdisable-fp-elim"
+// NO-OMIT-ALL3-NOT: "-momit-leaf-frame-pointer"
+
 void f0() {}
 void f1() { f0(); }