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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
