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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added reviewers: hans, rnk.
chandlerc added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: lib/CodeGen/CGCall.cpp:1739
       FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
-      FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
     }
----------------
tabloid.adroit wrote:
> chandlerc wrote:
> > This seems like an unrelated change?
> 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.
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.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:592-595
+  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);
----------------
tabloid.adroit wrote:
> chandlerc wrote:
> > 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.
> 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.
> 
> 
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).


================
Comment at: test/Driver/cl-options.c:177
 // 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
----------------
Do we want to also change behavior for the CL options? We should discuss this w/ the Windows folks at least....


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55915/new/

https://reviews.llvm.org/D55915