[PATCH] D92073: [CodeGen] Add text section prefix for COFF object file

[TaoPan via Phabricator via llvm-commits]

TaoPan marked an inline comment as done.
TaoPan added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:623
   if (const auto *F = dyn_cast<Function>(GO)) {
     if (Optional<StringRef> Prefix = F->getSectionPrefix()) {
+      raw_svector_ostream(Name) << '.' << *Prefix;
----------------
rnk wrote:
> This is out of scope for the patch, but why is it called a "section prefix" if it goes after the section name? ".text.hot._ZfooEv"
Without this modification, ".texthot._ZfooEv", no '.' between .text and hot, as the '.' was removed in CodeGenPrepare.cpp
Please have a look at I just added Summary, key reason is ELF use '.', COFF use '$'.


================
Comment at: llvm/test/CodeGen/X86/text-section-prefix.ll:1
+; RUN: llc -function-sections < %s
+
----------------
pengfei wrote:
> What's your checking here?
I added some description in Summary.

The line of output
   .section     .text,xxx
will be changed to
   .section     .text**$hot**,xxx
or
   .section     .text**$unlikely**,xxx
with this patch on Windows.


================
Comment at: llvm/test/CodeGen/X86/text-section-prefix.ll:1
+; RUN: llc -function-sections < %s
+
----------------
rnk wrote:
> TaoPan wrote:
> > pengfei wrote:
> > > What's your checking here?
> > I added some description in Summary.
> > 
> > The line of output
> >    .section     .text,xxx
> > will be changed to
> >    .section     .text**$hot**,xxx
> > or
> >    .section     .text**$unlikely**,xxx
> > with this patch on Windows.
> Please run this test in a few configurations. You can add two RUN lines like:
> ```
> ; RUN: llc -mtriple x86_64-linux-gnu %s -o - | FileCheck %s --check-prefix=ELF
> ; RUN: llc -mtriple x86_64-linux-gnu -unique-section-names=0 %s -o - | FileCheck %s --check-prefix=ELF-NOUNIQ
> ; RUN: llc -mtriple x86_64-windows-msvc %s -o - | FileCheck %s --check-prefix=MSVC
> ; RUN: llc -mtriple x86_64-windows-msvc %s -o - | FileCheck %s --check-prefix=MINGW
> ```
> 
> Add check lines for the section directives.
Thanks for your nice guidance!
I'll try your RUN lines.


================
Comment at: llvm/test/CodeGen/X86/text-section-prefix.ll:3
+
+define void @foo0(i1 zeroext %0) nounwind !section_prefix !0 {
+  ret void
----------------
MaskRay wrote:
> You may want some RUN configurations in `Transforms/CodeGenPrepare/X86/section.ll`
Thanks! I'll refer to this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92073