chandlerc requested changes to this revision.
chandlerc added reviewers: hans, rnk.
chandlerc added a comment.
This revision now requires changes to proceed.
The number of negations and such in the CC1 interface here and the attributes
makes all of these tests super confusing. Wonder if we should fix that before
making changes here.
Also adding folks to comment on the `cl.exe` driver mode and how it should
behave.
================
Comment at: lib/CodeGen/CGCall.cpp:1739
FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
- FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
}
----------------
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.
================
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);
----------------
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).
================
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
----------------
Do we want to also change behavior for the CL options? We should discuss this
w/ the Windows folks at least....
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
