Page MenuHomePhabricator

[CodeGen] Port basic block sections from ELF to COFF
Needs ReviewPublic

Authored by TaoPan on Mon, Mar 29, 12:31 AM.

Details

Summary

Ignore size check as COFF assembly doesn't supprt .size directive.
The override function getSectionForMachineBasicBlock will construct
exception section, cold section and general basic block section. This
patch constructs general basic block section only, the exception section
will be constructed in later basic block section Windows exception
handling patch, the cold section will be constructed in later machine
function splitter on Windows patch.

Signed-off-by: Pan, Tao <tao.pan@intel.com>

Diff Detail

Event Timeline

TaoPan created this revision.Mon, Mar 29, 12:31 AM
TaoPan requested review of this revision.Mon, Mar 29, 12:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 29, 12:31 AM
TaoPan updated this revision to Diff 333785.Mon, Mar 29, 1:59 AM

git rebase

tmsriram added inline comments.Mon, Mar 29, 10:41 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1711

Why is this set to always comdat? This should be comdat only if the function is comdat, right?

1712

COMDATSymName can be folded to be equal to MBB.getSymbol()->getName() here? Plus, you are not preserving the .text.hot prefix that the original function might get here. Is this future work? If the original function is named .text.hot.foo, the basic block will still be named .text.foo.__part.1 which is not right.

Plus, what about exception handling sections like ".eh.*"?

TaoPan added inline comments.Mon, Mar 29, 11:16 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1711

Thanks for pointing out comdat or not depending on the specific situation! I'll add comdat test to basic-block-sections.ll for making sure both comdat and not comdat work correctly. Then I'll answer your second question.

1712

Thanks! I'll redesign section name and comdat symbol name.
The text section with prefix "hot" and "unlikely" won't be constructed here, I added COFF text section prefix "hot" and "unlikely" in D92073. In ELF override function, also not handling text section with prefix "hot" and "unlikely".
The text section with prefix "split" will be constructed here, I plan to add related code in MFS COFF patch.
Also, exception handling section is future work that support basic block sections Windows COFF exception handling.

MaskRay added inline comments.Tue, Mar 30, 11:00 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1712

This is complex. PE-COFF has multiple COMDAT seletion kinds. I want to see a holistic plan how every component is going to be implemented.

TaoPan updated this revision to Diff 334386.Wed, Mar 31, 2:33 AM

Conditionally set COFF::IMAGE_SCN_LNK_COMDAT, add mtriple x86_64-windows-gnu support

tmsriram added inline comments.Wed, Mar 31, 3:39 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1712

The basic block should just mimic the COMDAT type of its containing function, is there a reason to do anything more with it here?

TaoPan updated this revision to Diff 334568.Wed, Mar 31, 5:45 PM

Simplify condition of setting COFF::IMAGE_SCN_LNK_COMDAT

TaoPan updated this revision to Diff 334569.Wed, Mar 31, 5:48 PM

git rebase

TaoPan added inline comments.Wed, Mar 31, 6:08 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1711

I referred to the existing COFF code (TargetLoweringObjectFileCOFF::SelectSectionForGlobal()), COFF::IMAGE_SCN_LNK_COMDAT of text section is set when setting -function-sections and the function is comdat.

TaoPan added a comment.Mon, Apr 5, 7:14 PM

Hi tmsriram, MaskRay,

I updated implementation and test. Could you please have a look?
As for multiple COMDAT seletion kinds of PE-COFF, is IMAGE_COMDAT_SELECT_NODUPLICATES wrong? Or need to set different COMDAT selection kind in some case?
As for a holistic plan how every component is going to be implemented, I'm developing

  1. Basic block sections Windows exception handling, I'm not sure whether need to handle exception handling sections like ELF sections ".eh.*" here, anyway, I'll make sure Windows exception handling work on basic block sections.
  2. MFS on Windows, I submitted D95209, it includes text section with prefix ".text$split" handling, it will be updated after landing this patch.

in turn.

Hi tmsriram, MaskRay,

I updated implementation and test. Could you please have a look?
As for multiple COMDAT seletion kinds of PE-COFF, is IMAGE_COMDAT_SELECT_NODUPLICATES wrong? Or need to set different COMDAT selection kind in some case?

I am not familiar with PE-COFF and I don't know the answer. FOR ELF, the basic block COMDAT type should be the same as its parent function and should also be in the same group.

As for a holistic plan how every component is going to be implemented, I'm developing

  1. Basic block sections Windows exception handling, I'm not sure whether need to handle exception handling sections like ELF sections ".eh.*" here, anyway, I'll make sure Windows exception handling work on basic block sections.

If it is easier for you to break this up, you can present exception handling in a separate patch with a clear "TODO:" at the appropriate places. That should be alright and is also easier to review.

  1. MFS on Windows, I submitted D95209, it includes text section with prefix ".text$split" handling, it will be updated after landing this patch.

in turn.

Great, please use the parent-child relationships to mark these patches.

  • Could you also please expand the summary of this patch? I would write what extra was needed to port this from ELF to PE-COFF.
  • Could you also talk about DebugInfo handling and CFI with PE-COFF and basic block sections? Are these format agnostic and have you verified that they do the right thing? These were the biggest challenges for us when doing this for ELF.

Appreciate your efforts towards porting this to PE-COFF and making it more generally available. Thanks!

penzn added a subscriber: penzn.Wed, Apr 14, 12:10 AM
TaoPan updated this revision to Diff 337368.Wed, Apr 14, 1:32 AM

Add TODO for constructing exception section and cold section later

TaoPan edited the summary of this revision. (Show Details)Wed, Apr 14, 1:33 AM

If it is easier for you to break this up, you can present exception handling in a separate patch with a clear "TODO:" at the appropriate places. That should be alright and is also easier to review.

I added "TODO:" for constructing exception section and cold section later.

Great, please use the parent-child relationships to mark these patches.

I marked parent-child relationships.

  • Could you also please expand the summary of this patch? I would write what extra was needed to port this from ELF to PE-COFF.

I expanded the summary, could you please have a review?

  • Could you also talk about DebugInfo handling and CFI with PE-COFF and basic block sections? Are these format agnostic and have you verified that they do the right thing? These were the biggest challenges for us when doing this for ELF.

No, I didn't verify DebugInfo handling and CFI with PE-COFF and basic block sections.

If it is easier for you to break this up, you can present exception handling in a separate patch with a clear "TODO:" at the appropriate places. That should be alright and is also easier to review.

I added "TODO:" for constructing exception section and cold section later.

Great, please use the parent-child relationships to mark these patches.

I marked parent-child relationships.

  • Could you also please expand the summary of this patch? I would write what extra was needed to port this from ELF to PE-COFF.

I expanded the summary, could you please have a review?

  • Could you also talk about DebugInfo handling and CFI with PE-COFF and basic block sections? Are these format agnostic and have you verified that they do the right thing? These were the biggest challenges for us when doing this for ELF.

No, I didn't verify DebugInfo handling and CFI with PE-COFF and basic block sections.

This is quite important to validate the correctness of the generated assembly. If you could please check that CFI and DebugInfo are sane before we take this patch forward? Let's get a temperature on how good these are before we decide how to take it forward.

This is quite important to validate the correctness of the generated assembly. If you could please check that CFI and DebugInfo are sane before we take this patch forward? Let's get a temperature on how good these are before we decide how to take it forward.

Thanks! I got the importance.
As for DebugInfo, I tried to add "--basic-block-sections=all" to RUN of llvm/test/DebugInfo/COFF/comdat.ll, text sections includes basic block text sections and CodeViewDebugInfo can be created correctly after

  1. apply this patch
  2. commenting out a Linux exception handling protocol dependent function which I plan to do in later basic block section Windows exception handling patch.

There are some non-blocking errors "error: all .cv_loc directives for a function must be in the same section" in llc stage, one error for each basic block, below is the first one.
f.__part.1: # %if.then
#DEBUG_VALUE: f:c <- $ecx
<unknown>:0: error: all .cv_loc directives for a function must be in the same section
.Ltmp3:

As for CFI, does it need be verified addition to llvm/test/DebugInfo/ tests? If so, could you please guide me how CFI of basic block sections is verified on Linux?

TaoPan updated this revision to Diff 337641.Thu, Apr 15, 12:02 AM

Simplify text section name code

TaoPan updated this revision to Diff 337670.Thu, Apr 15, 2:08 AM

git rebase