Page MenuHomePhabricator

[CodeGen] Port Machine Function Splitter from ELF to COFF
Needs ReviewPublic

Authored by TaoPan on Jan 22 2021, 12:16 AM.

Details

Summary

Unlike ELF that the name of MCSymbol is combined section name and symbol
name, the name of MCSymbol of COFF doesn't include section name, for
distinguishing with non-split MCSymbol, add BBSectionsColdTextPrefix to
name of MCSymbol.

Diff Detail

Unit TestsFailed

TimeTest
370 msx64 debian > LLVM.CodeGen/X86::wineh-machine-function-splitter.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/X86/wineh-machine-function-splitter.ll -mtriple=x86_64-windows-msvc -split-machine-functions -function-sections | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-mc -triple=x86_64-windows-msvc -filetype=obj - | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-readobj -S --sd --sr -u - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/X86/wineh-machine-function-splitter.ll
180 msx64 windows > LLVM.CodeGen/X86::wineh-machine-function-splitter.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-2\llvm-project\premerge-checks\build\bin\llc.exe < C:\ws\w16c2-2\llvm-project\premerge-checks\llvm\test\CodeGen\X86\wineh-machine-function-splitter.ll -mtriple=x86_64-windows-msvc -split-machine-functions -function-sections | c:\ws\w16c2-2\llvm-project\premerge-checks\build\bin\llvm-mc.exe -triple=x86_64-windows-msvc -filetype=obj - | c:\ws\w16c2-2\llvm-project\premerge-checks\build\bin\llvm-readobj.exe -S --sd --sr -u - | c:\ws\w16c2-2\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-2\llvm-project\premerge-checks\llvm\test\CodeGen\X86\wineh-machine-function-splitter.ll

Event Timeline

TaoPan created this revision.Jan 22 2021, 12:16 AM
TaoPan requested review of this revision.Jan 22 2021, 12:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2021, 12:16 AM

Could you please have a review?

Could you please give more details about COFF versus ELF differences with regards to

  1. DebuInfo Handling - Separating out a function into a separate section requires that debug info use dwarf ranges.
  2. CFI handling

if there are any. I am not familiar with COFF. Thanks for doing this!

llvm/lib/CodeGen/BasicBlockSections.cpp
83–84

Maybe fix the comment too.

TaoPan updated this revision to Diff 319183.Jan 25 2021, 6:21 PM

Fix name of comment of BBSectionsColdTextPrefix

Thanks for your review comment!
I don’t have any more detail information about COFF versus ELF difference with regards to DebugInfo and CFI handling.
I used this patch to build my c micro case, workable.
Is it ok that improves this feature by issue driven as it’s default off?
I’ll try to enable this feature to build some complex application, e.g. chromium windows, improve this feature if there are any issues.

llvm/lib/CodeGen/BasicBlockSections.cpp
83–84

Fixed, thanks!

snehasish requested changes to this revision.Jan 26 2021, 10:42 AM

IMHO we should not ship the split functions feature targeting COFF without more testing. The machine function splitter relies on basic block sections and we should ensure that the debug information/call frame information generated by basic block sections is appropriate for COFF based consumers. Perhaps we can start by extending the existing CFI and DebugInfo tests in llvm/test/CodeGen/X86/cfi-basic-block-sections-1.ll and llvm/test/DebugInfo/X86/basic-block-sections_1.ll (in a separate patch).

We are also very interested in having this work on windows targets such as Chromium so your initiative here is appreciated!

This revision now requires changes to proceed.Jan 26 2021, 10:42 AM

Thanks for your guidance of the existing CFI and DebugInfo tests, I'll add COFF tests.

TaoPan updated this revision to Diff 331770.Mar 18 2021, 11:44 PM

git rebase

TaoPan updated this revision to Diff 331771.Mar 18 2021, 11:52 PM

Use comdat name for split text section

TaoPan updated this revision to Diff 331772.Mar 18 2021, 11:56 PM

Add MFS Windows exception handling testcase

TaoPan updated this revision to Diff 331773.Mar 18 2021, 11:58 PM

Use same triple option format

Harbormaster completed remote builds in B94622: Diff 331773.
Harbormaster completed remote builds in B94621: Diff 331772.
TaoPan updated this revision to Diff 331790.Mar 19 2021, 1:12 AM

Fix llvm-mc option -mtriple -> -triple

Hi Tao, since it looks like you are making progress on this! I would suggest adding the Windows exception handling test for basic block sections instead of MFS in particular. It will simplify the test since you don't need profile information and we will get better confidence for the Propeller framework overall. It would be equivalent to this test: llvm/test/CodeGen/X86/basic-block-sections-eh.ll

Thanks Snehasish! I got the simplicity and value of Windows exception handling test for basic block sections. Is it ok adding llvm/test/CodeGen/X86/basic-block-sections-wineh.ll in a separate patch, as running the test won't execute the source code in this patch?

Yes, definitely adding it in a separate patch is a good idea :) Lets submit that patch first, before we submit with this one (otherwise any large binary generated by this patch is probably going to be broken).
It should include the necessary code changes for windows unwind info. I think the code modifications for that change should be in files: llvm/llvm-project/llvm/lib/MC/MCWin64EH.cpp and llvm/llvm-project/llvm/lib/CodeGen/AsmPrinter/WinException.cpp (this one will have changes for C++ programs with exceptions enabled).

penzn added a subscriber: penzn.Apr 14 2021, 12:10 AM