Page MenuHomePhabricator

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

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

Details

Summary

Ignore size check as COFF assembly doesn't supprt .size directive.
The override function getSectionForMachineBasicBlock will construct
cold section and general basic block section. This patch constructs
general basic block section only, 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

There are a very large number of changes, so older changes are hidden. Show Older Changes
TaoPan requested review of this revision.Mar 29 2021, 12:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2021, 12:31 AM
TaoPan updated this revision to Diff 333785.Mar 29 2021, 1:59 AM

git rebase

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

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

1735

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.Mar 29 2021, 11:16 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1734

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.

1735

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.Mar 30 2021, 11:00 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1735

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.Mar 31 2021, 2:33 AM

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

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

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.Mar 31 2021, 5:45 PM

Simplify condition of setting COFF::IMAGE_SCN_LNK_COMDAT

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

git rebase

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

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.Apr 5 2021, 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.Apr 14 2021, 12:10 AM
TaoPan updated this revision to Diff 337368.Apr 14 2021, 1:32 AM

Add TODO for constructing exception section and cold section later

TaoPan edited the summary of this revision. (Show Details)Apr 14 2021, 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.Apr 15 2021, 12:02 AM

Simplify text section name code

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

git rebase

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:

Would you be able to paste the entire assembly with cv loc directives? I can try to take a look.

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?

Basically, this means your code won't assemble with CFI and that is a serious limitation. This is similar to what we did for ELF. All CFI directives should be within a cfi_startproc cfi_endproc directive as part of a single section. When you fragment a function into multiple sections, this breaks. We solved this by adding a separate startproc endproc directive for each section.

I have some suggestions for you to better understand the problem:

  • GCC supports function splitting with -freorder-blocks-and-partition. You can use GCC to split a function and see what it does to cv_loc directives.
  • D79978 is what we did with ELF, my guess is it is very similar. You just need to add a startproc endproc to every function and duplicate all the information to compute Call Frame Address.

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:

Would you be able to paste the entire assembly with cv loc directives? I can try to take a look.

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?

Basically, this means your code won't assemble with CFI and that is a serious limitation. This is similar to what we did for ELF. All CFI directives should be within a cfi_startproc cfi_endproc directive as part of a single section. When you fragment a function into multiple sections, this breaks. We solved this by adding a separate startproc endproc directive for each section.

I have some suggestions for you to better understand the problem:

  • GCC supports function splitting with -freorder-blocks-and-partition. You can use GCC to split a function and see what it does to cv_loc directives.
  • D79978 is what we did with ELF, my guess is it is very similar. You just need to add a startproc endproc to every function and duplicate all the information to compute Call Frame Address.

https://llvm.org/docs/Extensions.html#codeview-dependent

These aren't the CFI directives and I falsely assumed that. Stems from my misunderstanding of COFF, please take a note of this.

Would you be able to paste the entire assembly with cv loc directives? I can try to take a look.

As the entire assembly has 500 lines, I sent to you through mail, I'll do if you think paste part of them or entire assembly to here is better.

Basically, this means your code won't assemble with CFI and that is a serious limitation. This is similar to what we did for ELF. All CFI directives should be within a cfi_startproc cfi_endproc directive as part of a single section. When you fragment a function into multiple sections, this breaks. We solved this by adding a separate startproc endproc directive for each section.

I have some suggestions for you to better understand the problem:

  • GCC supports function splitting with -freorder-blocks-and-partition. You can use GCC to split a function and see what it does to cv_loc directives.
  • D79978 is what we did with ELF, my guess is it is very similar. You just need to add a startproc endproc to every function and duplicate all the information to compute Call Frame Address.

https://llvm.org/docs/Extensions.html#codeview-dependent

These aren't the CFI directives and I falsely assumed that. Stems from my misunderstanding of COFF, please take a note of this.

Thanks for your detail guidance! It's very helpful. I'll study what you did with ELF and codeview format.

Hi tmsriram, MaskRay,

I submitted D100735 and D101421 for enabling exception handling and CodeView DebugInfo for basic block sections on Windows, they are all depend on the override functions getSectionForMachineBasicBlock and getUniqueSectionForFunction in this patch, I added these dependent functions in those patches for passing arc unit test. I'll rebase those patches after the dependent functions are merged into llvm. Could you please review these 3 patches?

modimo added a subscriber: modimo.Apr 28 2021, 1:18 PM

For some added testing you can run MSVC's SEH tests to see that this works properly: https://github.com/microsoft/compiler-tests/tree/master/seh. I wouldn't be surprised if not all of these pass in the baseline but we certainly want to make sure the ones that are remain non-regressed.

TaoPan added a comment.EditedApr 28 2021, 7:17 PM

For some added testing you can run MSVC's SEH tests to see that this works properly: https://github.com/microsoft/compiler-tests/tree/master/seh. I wouldn't be surprised if not all of these pass in the baseline but we certainly want to make sure the ones that are remain non-regressed.

Thanks for guiding MSVC's SEH tests! I found the SEH tests use cl (not clang-cl) to compile the c test file https://github.com/microsoft/compiler-tests/blob/2fc7859d212d5fb59628eb21aec50dfb4b934146/seh/runtests.cmd#L8, I'll try to change cl to clang-cl and make sure non-regressed.

Thanks for guiding MSVC's SEH tests! I found the SEH tests use cl (not clang-cl) to compile the c test file https://github.com/microsoft/compiler-tests/blob/2fc7859d212d5fb59628eb21aec50dfb4b934146/seh/runtests.cmd#L8, I'll try to change cl to clang-cl and make sure non-regressed.

Sure, those are the tests run by MSVC for SEH verification which is why it uses cl. Clang-cl sounds good for testing.

Agree with @MaskRay I'd like to see a holistics plan/RFC on all the pieces that are needed to get this functionality. The functionality here is only tested when exceptions are actually propagated which is not common in LLVM test suite. You'll want to run and likely write very specific tests stressing the test matrix here of BBS + MFS + EH. Given all that, a larger plan that details how we can be confident that all the pieces have been accounted for and will be well tested is (IMO) imperative in getting this change reviewed and accepted.

Agree with @MaskRay I'd like to see a holistics plan/RFC on all the pieces that are needed to get this functionality. The functionality here is only tested when exceptions are actually propagated which is not common in LLVM test suite. You'll want to run and likely write very specific tests stressing the test matrix here of BBS + MFS + EH. Given all that, a larger plan that details how we can be confident that all the pieces have been accounted for and will be well tested is (IMO) imperative in getting this change reviewed and accepted.

Since this (the test matrix) is already done for ELF, would it be complete if the author added equivalent tests for COFF?

rnk added inline comments.Apr 29 2021, 12:59 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1735

After thinking about it a bit, I think the entry block should use the regular selection kind, and all of the auxilliary MBB sections should use IMAGE_COMDAT_SELECT_ASSOCIATIVE. They should be associated with the main function symbol, unless the function itself is associated with something else, in which case the BBs use that symbol.

This will ensure that if the main function section prevails, they are included, and if it does not prevail, they are discarded. Does that make sense?

Agree with @MaskRay I'd like to see a holistics plan/RFC on all the pieces that are needed to get this functionality. The functionality here is only tested when exceptions are actually propagated which is not common in LLVM test suite. You'll want to run and likely write very specific tests stressing the test matrix here of BBS + MFS + EH. Given all that, a larger plan that details how we can be confident that all the pieces have been accounted for and will be well tested is (IMO) imperative in getting this change reviewed and accepted.

Since this (the test matrix) is already done for ELF, would it be complete if the author added equivalent tests for COFF?

Good point, that sounds like a good avenue to pursue. @snehasish mentions some of these exact tests here https://reviews.llvm.org/D95209#2523227. Looks like there was some movement for these tests here: https://reviews.llvm.org/D96393 although it appears BBS might need Windows debuginfo support for testing. I'm chasing through a bunch of these diffs to figure out what the state of this is and I'm not quite certain what it is. @TaoPan do you have a plan you can share about the progress here so we can follow along?

Agree with @MaskRay I'd like to see a holistics plan/RFC on all the pieces that are needed to get this functionality. The functionality here is only tested when exceptions are actually propagated which is not common in LLVM test suite. You'll want to run and likely write very specific tests stressing the test matrix here of BBS + MFS + EH. Given all that, a larger plan that details how we can be confident that all the pieces have been accounted for and will be well tested is (IMO) imperative in getting this change reviewed and accepted.

Since this (the test matrix) is already done for ELF, would it be complete if the author added equivalent tests for COFF?

Good point, that sounds like a good avenue to pursue. @snehasish mentions some of these exact tests here https://reviews.llvm.org/D95209#2523227. Looks like there was some movement for these tests here: https://reviews.llvm.org/D96393 although it appears BBS might need Windows debuginfo support for testing. I'm chasing through a bunch of these diffs to figure out what the state of this is and I'm not quite certain what it is. @TaoPan do you have a plan you can share about the progress here so we can follow along?

Sorry for confusion! Let me summarize the history.

  1. I submitted D95209 for porting MFS to Windows COFF. @tmsriram and @snehasish mentioned CFI, DebugInfo and exception handling should be guaranteed, thanks for their help and guidance!
  2. I submitted D96393 for adding MFS DebugInfo test, as MFS COFF implementation code is in 1, it includes only Linux ELF test. I planed to extend MFS DebugInfo COFF test in 1 at that time. @snehasish guided me that add (MFS dependent) BBS not MFS DebugInfo test on Windows COFF, just like Linux ELF.
  3. I submitted this patch D99487 for supporting basic function of BBS on Windows COFF.
  4. I submitted D100735 for making sure Windows exception handling for BBS. As Linux ELF exception handling test llvm/test/CodeGen/X86/basic-block-sections-eh.ll is mainly testing throw & catch exception, so I referred https://docs.microsoft.com/en-us/cpp/cpp/structured-exception-handling-c-cpp?view=msvc-160 to generate ll test file.
  5. I submitted D101421 for making sure Windows CodeView DebugInfo for BBS. I used same C code of llvm/test/CodeGen/X86/cfi-basic-block-sections-1.ll introduced by D79978 that is CFI DebugInfo for BBS on Linux ELF to generate ll test file.

So the dependence chain is: 1 depends on 3 depends on 4 and 5. 2 will be abandoned.
Could you please review the upper progress? Any question is welcome.

Thanks all!
I'll take leave between 5.1~5.6 for national holiday and personal affair, I'll response @rnk 's review comment and other further review comment after then.

TaoPan updated this revision to Diff 343595.May 7 2021, 12:31 AM

Change selection of the auxilliary MBB sections to IMAGE_COMDAT_SELECT_ASSOCIATIVE, git rebase, fix comdat name issue

TaoPan updated this revision to Diff 343616.May 7 2021, 2:15 AM

git rebase

TaoPan added inline comments.May 7 2021, 2:21 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1735

Thanks! I think set entry block sections as regular IMAGE_COMDAT_SELECT_NODUPLICATES and set the auxilliary MBB sections as IMAGE_COMDAT_SELECT_ASSOCIATIVE is fine. I changed.

mstorsjo added inline comments.May 7 2021, 2:23 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1735

@rnk - I'm not quite familiar with the concrete implications of this work here, but would this need to be mapped to the GNU style comdats, for compatibility with ld.bfd for mingw targets? (LLD itself works fine with proper associative comdats though.)

rnk added inline comments.May 7 2021, 12:48 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1735

I think basic block sections are kind of in the same category as LTO: it's a very sophisticated optimization feature, and it's probably OK if it's LLD-only. I think it also requires special linker support that might make it LLD-only, but I've forgotten the details.

It also has potentially very high object file size overhead, and it will be important to do everything possible to minimize that object file size overhead. Long gnu-style section names for every basic block section are likely to make the feature unusably slow, so maybe it's worth removing these "if mingw" conditionals. We can leave behind a comment by the use of IMAGE_COMDAT_SELECT_ASSOCIATIVE indicating that gnu-style section names are not needed because the feature is only designed to work with LLD.

mstorsjo added inline comments.May 7 2021, 1:37 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1735

Thanks for the clarification! Leaving the feature as LLD-only (or in other terms, requiring a conforming COMDAT implementation) sounds good to me.

TaoPan updated this revision to Diff 343804.May 7 2021, 8:20 PM

Remove if mingw conditionals of MBB sections.

TaoPan edited the summary of this revision. (Show Details)May 7 2021, 8:23 PM
TaoPan updated this revision to Diff 343805.May 7 2021, 8:46 PM

git rebase for triggering auto test

TaoPan updated this revision to Diff 343810.May 7 2021, 10:08 PM

git rebase for triggering auto test

The x64 debian failed was introduced by D101797 and is being fixed by https://reviews.llvm.org/rG72bd0116e3a1a70fb52fc47c056349b290ce2204

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1735

Thanks for discussion between @mstorsjo and @rnk !
I removed "if mingw" conditionals.

TaoPan updated this revision to Diff 344658.May 11 2021, 10:02 PM

git rebase

Thanks for guiding MSVC's SEH tests! I found the SEH tests use cl (not clang-cl) to compile the c test file https://github.com/microsoft/compiler-tests/blob/2fc7859d212d5fb59628eb21aec50dfb4b934146/seh/runtests.cmd#L8, I'll try to change cl to clang-cl and make sure non-regressed.

Sure, those are the tests run by MSVC for SEH verification which is why it uses cl. Clang-cl sounds good for testing.

Sorry for late feedback of this comment!
I checked the microsoft SEH tests with

  1. cl.exe

    a. x4ptcu.c: build error
  2. clang-cl.exe + lld linker

    a. x4ptcu.c: build error

    b. seh0015.c, seh0017.c: build crash

    c. seh0034.c, seh0036.c, seh0041~0043.c, seh0048~0050.c, another build crash

    d. seh0020.c, seh0025, seh0026: build error

    e. sehframes.cpp: build pass, run dead loop
  3. 2 + BBS related patches

    Test results are same with 2.
  4. 3 + option -Xclang -fbasic-block-sections=all -Xclang -funique-basic-block-section-names

    a. All tests build crash, report lld link error

    lld/COFF/InputFiles.cpp, ObjFile::readAssociativeDefinition(), error(toString(this) + ": associative comdat " + name + " (sec " + Twine(sectionNumber) + ") has invalid reference to section " + parentName + " (sec " + Twine(parentIndex) + ")");

I'm investigating 4.a.

I checked the microsoft SEH tests with

  1. cl.exe

    a. x4ptcu.c: build error
  2. clang-cl.exe + lld linker

    a. x4ptcu.c: build error

    b. seh0015.c, seh0017.c: build crash

    c. seh0034.c, seh0036.c, seh0041~0043.c, seh0048~0050.c, another build crash

    d. seh0020.c, seh0025, seh0026: build error

    e. sehframes.cpp: build pass, run dead loop

Please file bugs for these and if you can bucket the failures that would be even better. Thanks!