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

[Yuanfang Chen via Phabricator via cfe-commits]

tabloid.adroit marked 3 inline comments as done.
tabloid.adroit added inline comments.


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


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



================
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
----------------
chandlerc wrote:
> Do we want to also change behavior for the CL options? We should discuss this w/ the Windows folks at least....
Sure. It would be great to have them to confirm.



Repository:
  rC Clang

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

https://reviews.llvm.org/D55915