This revision was automatically updated to reflect the committed changes.
Closed by commit rL280355: [Frontend] Fix mcount inlining bug (authored by
honggyu.kim).
Changed prior to commit:
https://reviews.llvm.org/D22666?vs=68954&id=69986#toc
Repository:
rL LLVM
https://reviews.llvm.org/D226
honggyu.kim added a reviewer: compnerd.
honggyu.kim removed a subscriber: compnerd.
honggyu.kim updated this revision to Diff 68954.
honggyu.kim added a comment.
I have fixed SOH character problem by https://reviews.llvm.org/D23792. I've
verified that mcount.c and gnu-mcount.c tests are passed n
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#513465, @compnerd wrote:
> No, the inserted character is a literal SOH, not the string "\01".
I don't know why the string is passed in a different way but '\01' is shown in
IR previously as below
$ clang -target armv7-unknown-n
compnerd added a comment.
No, the inserted character is a literal SOH, not the string "\01".
https://reviews.llvm.org/D22666
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#509904, @compnerd wrote:
> Theres a frontend mangler and a backend mangler. You should try this on
> Windows GNU environments :-).
Thanks for the confirmation :)
If so, would it be okay if I put one more backslash for each mcount
compnerd added a comment.
Theres a frontend mangler and a backend mangler. You should try this on
Windows GNU environments :-).
https://reviews.llvm.org/D22666
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/m
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#506884, @compnerd wrote:
> The `\01` is to prevent the mangler from touching the function name. If you
> noticed the check tags in the quoted test, some targets expect it to be
> decorated and others do not. What exactly do you m
honggyu.kim added a comment.
I see the difference now.
Below is the originally generated IR without this patch. It shows `\01`
clearly in `call void @"\01__gnu_mcount_nc"()`.
$ clang -target armv7-unknown-none-eabi -pg -meabi gnu -S -emit-llvm -o -
test-mcount.c
; ModuleID = 'mcount.c'
s
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#506884, @compnerd wrote:
> The `\01` is to prevent the mangler from touching the function name. If you
> noticed the check tags in the quoted test, some targets expect it to be
> decorated and others do not. What exactly do you m
compnerd added a comment.
The `\01` is to prevent the mangler from touching the function name. If you
noticed the check tags in the quoted test, some targets expect it to be
decorated and others do not. What exactly do you mean that `lit` has a strings
matching with `\01`? Its not a string `
honggyu.kim added a comment.
Hi Renato and Saleem,
I found one more testcase that has to be changed. It's in
`llvm/tools/clang/test/Frontend/gnu-mcount.c`.
But strangely, some cases provide mcount name with prefix '\01' such as below:
class ARMTargetInfo : public TargetInfo {
...
public
honggyu.kim updated the summary for this revision.
honggyu.kim updated this revision to Diff 66276.
honggyu.kim added a comment.
I've updated the testcase again it adds 2 more tests for -O2 and without -pg.
Please have a look at it.
$ llvm-lit mcount.c
-- Testing: 1 tests, 1 threads --
PASS
hfinkel added a comment.
In https://reviews.llvm.org/D22666#500338, @honggyu.kim wrote:
> In https://reviews.llvm.org/D22666#500329, @hfinkel wrote:
>
> > In this case, I think that making a simple test (changing the current test
> > to be) like test/CodeGen/stackrealign.c would be fine. If you
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#500329, @hfinkel wrote:
> In this case, I think that making a simple test (changing the current test to
> be) like test/CodeGen/stackrealign.c would be fine. If you have any
> questions, please feel free to ask.
Thanks. Please co
hfinkel added a comment.
In https://reviews.llvm.org/D22666#500328, @honggyu.kim wrote:
> In https://reviews.llvm.org/D22666#500327, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D22666#500326, @honggyu.kim wrote:
> >
> > > Should we also modify clang/test/CodeGen/mcount.c as well? I'm not
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#500327, @rjmccall wrote:
> In https://reviews.llvm.org/D22666#500326, @honggyu.kim wrote:
>
> > Should we also modify clang/test/CodeGen/mcount.c as well? I'm not
> > actually familiar with test infra.
>
>
> Yes, you'll need to mod
rjmccall added a comment.
In https://reviews.llvm.org/D22666#500326, @honggyu.kim wrote:
> Should we also modify clang/test/CodeGen/mcount.c as well? I'm not actually
> familiar with test infra.
Yes, you'll need to modify it to test for the attribute instead.
https://reviews.llvm.org/D22666
honggyu.kim added a comment.
Should we also modify clang/test/CodeGen/mcount.c as well? I'm not actually
familiar with test infra.
$ cat llvm/tools/clang/test/CodeGen/mcount.c
// RUN: %clang_cc1 -pg -triple i386-unknown-unknown -emit-llvm -o - %s |
FileCheck %s
// RUN: %clang_cc1 -pg -tr
honggyu.kim marked 4 inline comments as done.
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#499583, @hfinkel wrote:
> Comments about the comment, but otherwise, LGTM.
Just updated the comment as you mentioned. Thanks for correcting my English :)
https://reviews.llvm.org/D226
honggyu.kim updated the summary for this revision.
honggyu.kim updated this revision to Diff 66072.
https://reviews.llvm.org/D22666
Files:
lib/CodeGen/CodeGenFunction.cpp
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/
rjmccall added a comment.
LGTM, too.
https://reviews.llvm.org/D22666
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.
Comments about the comment, but otherwise, LGTM.
Comment at: lib/CodeGen/CodeGenFunction.cpp:789
@@ -796,1 +788,3 @@
+ // Since emitting mcount call here impacts optimiza
honggyu.kim added a comment.
I just wrote how I tested this in the bugzilla report page below:
https://llvm.org/bugs/show_bug.cgi?id=28660
https://reviews.llvm.org/D22666
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/
honggyu.kim updated this revision to Diff 65879.
honggyu.kim added a comment.
I've updated the diff that can be working with https://reviews.llvm.org/D22825.
I appreciate your work in backend, Hal.
Please correct me if I'm wrong.
https://reviews.llvm.org/D22666
Files:
lib/CodeGen/CodeGenFun
hfinkel added a comment.
In https://reviews.llvm.org/D22666#497085, @honggyu.kim wrote:
> In https://reviews.llvm.org/D22666#496355, @hfinkel wrote:
>
> > Here's a patch for the backend part: https://reviews.llvm.org/D22825 - If
> > this makes sense to everyone, we can change the frontend to add
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#496355, @hfinkel wrote:
> Here's a patch for the backend part: https://reviews.llvm.org/D22825 - If
> this makes sense to everyone, we can change the frontend to add the
> corresponding attribute instead of inserting the function c
hfinkel added a comment.
In https://reviews.llvm.org/D22666#496218, @hfinkel wrote:
> In https://reviews.llvm.org/D22666#495978, @honggyu.kim wrote:
>
> > In https://reviews.llvm.org/D22666#493708, @rjmccall wrote:
> >
> > > Note that the presence of the mcount call itself will significantly
> >
hfinkel added a comment.
In https://reviews.llvm.org/D22666#495978, @honggyu.kim wrote:
> In https://reviews.llvm.org/D22666#493708, @rjmccall wrote:
>
> > Note that the presence of the mcount call itself will significantly impact
> > inlining and, really, the entire optimization pipeline. Havi
honggyu.kim added a comment.
One more problem is that -finstrument-functions has the same inlining problem.
The code in `llvm/tools/clang/lib/CodeGen/CodeGenFunction.cpp` shows that.
void CodeGenFunction::StartFunction(GlobalDecl GD,
QualType RetTy,
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#493708, @rjmccall wrote:
> Note that the presence of the mcount call itself will significantly impact
> inlining and, really, the entire optimization pipeline. Having this be a
> late pass seems more in keeping with what I assume
rjmccall added a comment.
Note that the presence of the mcount call itself will significantly impact
inlining and, really, the entire optimization pipeline. Having this be a late
pass seems more in keeping with what I assume is a goal that this
instrumentation doesn't drastically change the ge
hfinkel added a comment.
In https://reviews.llvm.org/D22666#493706, @honggyu.kim wrote:
> In https://reviews.llvm.org/D22666#493109, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D22666#493026, @hfinkel wrote:
> >
> > > What's the actual desired behavior here? Should the inliner strip out
>
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#493109, @rjmccall wrote:
> In https://reviews.llvm.org/D22666#493026, @hfinkel wrote:
>
> > What's the actual desired behavior here? Should the inliner strip out calls
> > to mcount() as it inlines?
>
>
> I think they basically just
rjmccall added a comment.
In https://reviews.llvm.org/D22666#493026, @hfinkel wrote:
> What's the actual desired behavior here? Should the inliner strip out calls
> to mcount() as it inlines?
I think they basically just want a late pass (as in immediately prior to
codegen) to insert the mcoun
hfinkel added a subscriber: hfinkel.
hfinkel added a comment.
What's the actual desired behavior here? Should the inliner strip out calls to
mcount() as it inlines?
https://reviews.llvm.org/D22666
___
cfe-commits mailing list
cfe-commits@lists.llvm
honggyu.kim added a comment.
Hi John and Hans,
I don't know if I found right reviewers for this patch, but could you please
have a look at it?
I found this problem while I was trying to profile a binary that is compiled
with -pg and -O2 as I reported in bugzilla.
https://llvm.org/bugs/show_bug.
36 matches
Mail list logo
