Page MenuHomePhabricator

[CodeGen] emit CG profile for COFF object file
AcceptedPublic

Authored by zequanwu on Wed, Sep 16, 7:06 PM.

Details

Summary

I forgot to add emission of CG profile for COFF object file, when adding the support(https://reviews.llvm.org/D81775)

Diff Detail

Event Timeline

zequanwu created this revision.Wed, Sep 16, 7:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 16, 7:06 PM
zequanwu requested review of this revision.Wed, Sep 16, 7:06 PM
zequanwu updated this revision to Diff 292686.Thu, Sep 17, 6:08 PM

test case.

zequanwu edited the summary of this revision. (Show Details)Thu, Sep 17, 6:08 PM
rnk accepted this revision.Fri, Sep 18, 10:48 AM

lgtm!

I see the assembler support is already there, looks like it works.

This revision is now accepted and ready to land.Fri, Sep 18, 10:48 AM
This revision was automatically updated to reflect the committed changes.
rnk added inline comments.Tue, Sep 22, 2:31 PM
llvm/lib/Target/TargetLoweringObjectFile.cpp
164

I had to revert this change in rG90242caca2074dab5a9b76e5bc36d9fafd2179a7. I think the fix might be to return null if the call graph edge target is marked dllimport here.

zequanwu added inline comments.Tue, Sep 22, 2:43 PM
llvm/lib/Target/TargetLoweringObjectFile.cpp
164

I was thinking about a discussion in https://reviews.llvm.org/D81775?id=270540#inline-751699, which might be the cause. I was trying to do the same as ELF. Not sure if that's the cause.

zequanwu reopened this revision.Wed, Sep 23, 4:57 PM
This revision is now accepted and ready to land.Wed, Sep 23, 4:57 PM
zequanwu updated this revision to Diff 293899.Wed, Sep 23, 5:09 PM

Update. Don't emit CGProfileEntry if function has dll import storage class.

rnk accepted this revision.Thu, Sep 24, 2:20 PM

lgtm

llvm/lib/Target/TargetLoweringObjectFile.cpp
165

Thanks! You can see the diff here if you are curious:
https://reviews.llvm.org/D87811?vs=292846&id=293899#toc

This revision was automatically updated to reflect the committed changes.
zequanwu reopened this revision.Mon, Sep 28, 3:04 PM
This revision is now accepted and ready to land.Mon, Sep 28, 3:04 PM
zequanwu updated this revision to Diff 294817.EditedMon, Sep 28, 3:06 PM

For symbols haven't seen, just set them to external not weak external.

Previously I did it that way, because http://llvm.org/docs/Extensions.html#sht-llvm-call-graph-profile-section-call-graph-profile says

If either symbol is undefined, then that symbol is defined as if .weak symbol has been written at the end of the file.

zequanwu added inline comments.Mon, Sep 28, 3:09 PM
llvm/lib/MC/MCWinCOFFStreamer.cpp
344 ↗(On Diff #294817)

It's meaningless to set symbol to WeakExternal and External at same time here. At the end, the symbol just become WeakExternal.

rnk added a comment.Mon, Sep 28, 4:53 PM

Can you remove the weak external marking in a separate change from relanding? It changes existing functionality of the .cg_profile directive, so it can be committed independently, which is nice for making changes as small and independent as possible.

zequanwu updated this revision to Diff 294839.Mon, Sep 28, 5:19 PM

Split changes on .cg_profile to another patch D88456.