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

[TaoPan via Phabricator via cfe-commits]

TaoPan added a comment.

Thanks MaskRay for your review comments!



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1752
+  else {
+    UniqueID = NextUniqueID++;
+    COMDATSymName = MBB.getParent()->getName();
----------------
MaskRay wrote:
> I think `UniqueID = NextUniqueID++;` is not needed.
I added comment (line 1752) to explain why add this line, this line referred to ELF implementation, please refer to https://github.com/llvm/llvm-project/blob/cb2a2ba8d64da5be3fac0ea90e406270e8a615bd/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#L993


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1756
+
+  // TODO: construct cold section in the case of section ID of MBB is
+  // MBBSectionID::ColdSectionID
----------------
MaskRay wrote:
> Can you fix the TODO in this patch?
It's indeed TODO. MMBSectionID::ColdSectionID will be handled in MFS on Windows COFF patch. I plan to re-base my previous MFS on Windows COFF patch (D95209) after this patch. I also explained this TODO in Summary.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1786
+  // COFF linker will not properly handle comdats otherwise.
+  if (getContext().getTargetTriple().isWindowsGNUEnvironment()) {
+    Name += '$';
----------------
MaskRay wrote:
> Is `isOSBinFormatCOFF()` more appropriate? Can windows-gnu use the optimization?
Windows-gnu can use the optimization. In the case of Windows-msvc, isOSBinFormatCOFF() is true, but the section name should not include COMDATSymName suffix.
This part referred to existing code https://github.com/llvm/llvm-project/blob/cb2a2ba8d64da5be3fac0ea90e406270e8a615bd/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#L1663
Thanks for this Windows-gnu comment! I reviewed Window-gnu related implementation and fixed Windows-gnu problem of TargetLoweringObjectFileCOFF::getSectionForMachineBasicBlock(). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99487