[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:1711
+      COFF::IMAGE_SCN_CNT_CODE | COFF::IMAGE_SCN_MEM_EXECUTE |
+          COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_LNK_COMDAT,
+      SectionKind::getText(), COMDATSymName,
----------------
tmsriram wrote:
> Why is this set to always comdat? This should be comdat only if the function is comdat, right?
Thanks for pointing out comdat or not depending on the specific situation! I'll add comdat test to basic-block-sections.ll for making sure both comdat and not comdat work correctly. Then I'll answer your second question.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99487