[PATCH] D101311: Basic block sections for functions with implicit-section-name attribute

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added a comment.

LGTM with minor comments.



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:761
 
-  MCSectionELF *Section = getContext().getELFSection(
+  // With basic block sections, function entry must be made unique too. This
+  // works perfectly fine with section attribute or pragma section as the
----------------
rahmanl wrote:
> tmsriram wrote:
> > rahmanl wrote:
> > > Except may for chromium with its complex implementation detail here: https://chromium.googlesource.com/chromium/src/+/66.0.3359.158/base/memory/protected_memory.h
> > > Any thoughts on whether this would work with it (related bug: https://bugs.chromium.org/p/chromium/issues/detail?id=990942).
> > Section attributes work just fine with the patch. I can add another test.  This "," separated specific usage however does not seem to do anything with linux ELF.  The "," is just treated as another character. 
> It's probably fine because we only do unique sections for code (and only with the `-basic-block-sections` option) and the section flags are always the same 'ax'. Anyhow, we can look at this when we build chromium with basic block sections.
This sentence sounds more relevant to the callsite of the function. Here maybe we can simply say "Get a unique ID if forced to emit unique sections. [the rest of the comment]"


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:761-763
+  // With basic block sections, function entry must be made unique too. This
+  // works perfectly fine with section attribute or pragma section as the
+  // sections with the same name are grouped together by the assembler.
----------------
tmsriram wrote:
> rahmanl wrote:
> > Except may for chromium with its complex implementation detail here: https://chromium.googlesource.com/chromium/src/+/66.0.3359.158/base/memory/protected_memory.h
> > Any thoughts on whether this would work with it (related bug: https://bugs.chromium.org/p/chromium/issues/detail?id=990942).
> Section attributes work just fine with the patch. I can add another test.  This "," separated specific usage however does not seem to do anything with linux ELF.  The "," is just treated as another character. 
It's probably fine because we only do unique sections for code (and only with the `-basic-block-sections` option) and the section flags are always the same 'ax'. Anyhow, we can look at this when we build chromium with basic block sections.


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-pragma-sections.ll:5
+; RUN: echo "!!2" >> %t.order.txt
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t.order.txt | FileCheck %s --check-prefix=ORDER
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t.order.txt | FileCheck %s --check-prefix=ORDER
----------------
nit: This is not really ordering basic block sections. Maybe "list" is a better name?


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

https://reviews.llvm.org/D101311