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

[TaoPan via Phabricator via llvm-commits]

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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99487