[PATCH] D99487: [CodeGen] Port basic block sections from ELF to COFF

Fri Jul 30 09:35:54 PDT 2021

rnk added inline comments.

================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1712
+          COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_LNK_COMDAT,
+      SectionKind::getText(), COMDATSymName,
+      COFF::IMAGE_COMDAT_SELECT_NODUPLICATES, UniqueID);
----------------
TaoPan wrote:
> TaoPan wrote:
> > TaoPan wrote:
> > > mstorsjo wrote:
> > > > rnk wrote:
> > > > > mstorsjo wrote:
> > > > > > TaoPan wrote:
> > > > > > > rnk wrote:
> > > > > > > > tmsriram wrote:
> > > > > > > > > MaskRay wrote:
> > > > > > > > > > TaoPan wrote:
> > > > > > > > > > > tmsriram wrote:
> > > > > > > > > > > > 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.*"?
> > > > > > > > > > > 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.
> > > > > > > > > > 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.
> > > > > > > > > The basic block should just mimic the COMDAT type of its containing function, is there a reason to do anything more with it here?
> > > > > > > > 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?
> > > > > > > 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.
> > > > > > @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.)
> > > > > 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.
> > > > Thanks for the clarification! Leaving the feature as LLD-only (or in other terms, requiring a conforming COMDAT implementation) sounds good to me.
> > > Thanks for discussion between @mstorsjo and @rnk !
> > > I removed "if mingw" conditionals.
> > @rnk about the issue 4.a of https://reviews.llvm.org/D99487#2821343, I tried to use clang-cl.exe with bbs option and lld to build helloworld c program, meet IMAGE_COMDAT_SELECT_XXX related problem.
> > helloworld c program is general c demo program as below
> > $ cat helloworld.c
> > #include <stdio.h>
> > 
> > int main() {
> >    printf("hello world!\n");
> >    return 0;
> > }
> > $ clang-cl.exe helloworld.c -Xclang -fbasic-block-sections=all -Xclang -funique-basic-block-section-names -fuse-ld=lld
> > lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 25) has invalid reference to section .text (sec 25)
> > lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 26) has invalid reference to section .text (sec 26)
> > lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 27) has invalid reference to section .text (sec 27)
> > clang-cl: error: linker command failed with exit code 1 (use -v to see invocation)
> > 
> > These errors can be fixed by change selection of getSectionForMachineBasicBlock() from IMAGE_COMDAT_SELECT_ASSOCIATIVE to IMAGE_COMDAT_SELECT_ANY.
> > 
> > Then new error occurs.
> > $ clang-cl.exe helloworld.c -Xclang -fbasic-block-sections=all -Xclang -funique-basic-block-section-names
> > libcmt.lib(default_local_stdio_options.obj) : error LNK2005: __local_stdio_printf_options already defined in helloworld-1f7490.obj
> > libvcruntime.lib(undname.obj) : error LNK2005: __local_stdio_printf_options already defined in helloworld-1f7490.obj
> > helloworld.exe : fatal error LNK1169: one or more multiply defined symbols found
> > clang-cl: error: linker command failed with exit code 1169 (use -v to see invocation)
> > 
> > This error can be fixed by change selection of getUniqueSectionForFunction() from IMAGE_COMDAT_SELECT_NODUPLICATES to IMAGE_COMDAT_SELECT_ANY.
> > Is it acceptable change selection of getSectionForMachineBasicBlock() and getUniqueSectionForFunctio() to IMAGE_COMDAT_SELECT_ANY?
> > 
> @rnk The last sentence of IMAGE_COMDAT_SELECT_ASSOCIATIVE's Description: The section association chain must eventually come to a COMDAT section that doesn't have IMAGE_COMDAT_SELECT_ASSOCIATIVE set. The upper "lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 25) has invalid reference to section .text (sec 25)" is caused by the association chain (only has BB text section) doesn't have a COMDAT section that doesn't have IMAGE_COMDAT_SELECT_ASSOCIATIVE set. I think the association chain should be per section chain, not per function chain, like:
>    entry block text section (default one, not IMAGE_COMDAT_SELECT_ASSOCIATIVE) -> data/debug/.. section if has (IMAGE_COMDAT_SELECT_ASSOCIATIVE)
>    BB section 1 (default one, not IMAGE_COMDAT_SELECT_ASSOCIATIVE) -> data/debug/... section if has (IMAGE_COMDAT_SELECT_ASSOCIATIVE)
>    BB section 2 (default one, not IMAGE_COMDAT_SELECT_ASSOCIATIVE) -> data/debug/... section if has (IMAGE_COMDAT_SELECT_ASSOCIATIVE)
>    ...
> So I changed BB text section's select to IMAGE_COMDAT_SELECT_NODUPLICATES.
> 
> As for the upper "clang-cl: error: linker command failed with exit code 1169 (use -v to see invocation)", I tried again with lld linker.
> $ clang-cl.exe helloworld.c -Xclang -fbasic-block-sections=all -Xclang -funique-basic-block-section-names -fuse-ld=lld
> lld-link: error: duplicate symbol: __local_stdio_printf_options
> >>> defined at C:\Users\taopan\AppData\Local\Temp\helloworld-b851d7.obj
> >>> defined at libcmt.lib(default_local_stdio_options.obj)
> 
> lld-link: error: duplicate symbol: __local_stdio_printf_options
> >>> defined at C:\Users\taopan\AppData\Local\Temp\helloworld-b851d7.obj
> >>> defined at libvcruntime.lib(undname.obj)
> clang-cl: error: linker command failed with exit code 1 (use -v to see invocation)
> 
> Duplication is between helloworld obj and system libcmt.lib(default_local_stdio_options.obj), so I changed select of entry block text section to IMAGE_COMDAT_SELECT_ANY.
> How do you think of these two modifications?
IIUC this error looks like the entry block was mistakenly associated with itself:
 "lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 25) has invalid reference to section .text (sec 25)

So, section 25 is associated with section 25, a self-reference, creating a loop, not a chain. As I said earlier, the entry block must use the "regular" selection type. Whatever LLVM would use today. The cold blocks should be associated with the entry block.

Alternatively, if the function itself is associative, it's equivalent to associate the cold blocks directly with the global that the function is associated with.

I don't like the idea of BBS arbitrarily making the entry block use `IMAGE_COMDAT_SELECT_ANY`. It should be using whatever selection LLVM would have chosen for it without BBS enabled.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99487/new/

https://reviews.llvm.org/D99487