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

[Yuanfang Chen via Phabricator via cfe-commits]

tabloid.adroit marked 2 inline comments as done.
tabloid.adroit added a comment.

Thanks for the review! Comments inline.



================
Comment at: lib/CodeGen/CGCall.cpp:1739
       FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
-      FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
     }
----------------
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.


================
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);
----------------
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.




Repository:
  rC Clang

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

https://reviews.llvm.org/D55915