[PATCH] D56353: Replace cc1 options '-mdisable-fp-elim' and '-momit-leaf-frame-pointer' with'-mframe-pointer='

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Wow, thanks for the cleanups. This is much easier to read as a consequence. And sorry it took a while to get you another round of review. Comments below.



================
Comment at: include/clang/Driver/CC1Options.td:270
+def mframe_pointer_EQ : Joined<["-"], "mframe-pointer=">,
+  HelpText<"Specify effect of frame pointer elimination optimization (all,non-leaf,none)">, Values<"all,non-leaf,none">;
 def mdisable_tail_calls : Flag<["-"], "mdisable-tail-calls">,
----------------
Should say more `Specify which frame pointers to retain (all, non-leaf, none)`.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:574
 
-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);
-
-  if (Args.hasArg(options::OPT_pg))
-    return true;
-
-  return useFramePointerForTargetByDefault(Args, Triple);
-}
+static StringRef DetermineFramePointer(const ArgList &Args,
+                                       const llvm::Triple &Triple) {
----------------
Keep LLVM coding convention naming pattern please.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:576-579
+  Arg *FP = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+                            options::OPT_fomit_frame_pointer);
+  Arg *LeafFP = Args.getLastArg(options::OPT_mno_omit_leaf_frame_pointer,
+                                options::OPT_momit_leaf_frame_pointer);
----------------
This still doesn't make sense to me...

If the user specifies `-fomit-frame-point` or `-fno-omit-frame-pointer` *after* `-momit-leaf-frame-pointer` or `-mno-omit-leaf-frame-pointer`, then that last flag should win...

I think you need to first get the "base" FP state by checking the main two flags. Then you need to get the "last" FP state by checking *all four flags*. When the last flag is a leaf flag, then the state is determined by the base + the last. When the last flag is not one of the leaf flags, then the last flag fully specifies the result.

I think you can also process these variables into something simpler to test below, essentially handling all the matching logic in one place.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56353