[PATCH] D64294: [Driver] Consolidate shouldUseFramePointer() and shouldUseLeafFramePointer()

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked 5 inline comments as done.
MaskRay added inline comments.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:579
 
-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 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 (Args.hasArg(options::OPT_pg))
-    return true;
-
-  if (Triple.isPS4CPU())
-    return false;
-
-  return useFramePointerForTargetByDefault(Args, Triple);
+static FramePointerKind getFramePointerKind(const ArgList &Args,
+                                            const llvm::Triple &Triple) {
----------------
ychen wrote:
> `getFramePointerKind` -> `decideFramePointerKind` / `determineFramePointerKind` ? 
This is a pure function. I think it is fine to use `get`. This is also consistent with several other `get*` in this file.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:585
+      (A && A->getOption().matches(options::OPT_fno_omit_frame_pointer)) ||
+      (!(A && A->getOption().matches(options::OPT_fomit_frame_pointer)) &&
+       (Args.hasArg(options::OPT_pg) ||
----------------
ychen wrote:
> MaskRay wrote:
> > ychen wrote:
> > > 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;
> > > ```
> > 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.
> 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;
> }
> ```
> 
> 
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64294