TaoPan 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: > > 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? 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
