TaoPan added a comment. @rnk Thanks for your review comments! Could you please help to review my reply and new modification? @MaskRay Could you please also help to review?
================ 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); ---------------- rnk wrote: > 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. The section association chain consists of multiple sections with **same** comdat name. In the case of BBS and -unique-basic-block-section-names, comdat names of entry block and BB sections are different (BB sections have suffix ".__part.x"), so BB sections can't be associated with the entry block. Previous issue "section 25 is associated with section 25" is caused by no COMDAT section (**with same name of BB section**) that doesn't have IMAGE_COMDAT_SELECT_ASSOCIATIVE set. I changed selection of entry block to same as without BBS enabled. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99487/new/ https://reviews.llvm.org/D99487 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
