[PATCH] D93876: Do not implicitly turn on function sections with basic block sections.

[Sriraman Tallam via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-blockaddress-taken.ll:10
 ; CHECK:         .text
+; CHECK:         .section .text.foo,"ax", at progbits
 ; CHECK-LABEL: foo:
----------------
dblaikie wrote:
> tmsriram wrote:
> > dblaikie wrote:
> > > tmsriram wrote:
> > > > dblaikie wrote:
> > > > > Why did this test change? It looks like it ended up with unique section names + function-sections enabled, but bb-unique-section-names not enabled? But that wasn't the case prior to this patch?
> > > > llc did not enable function sections implicitly with basic block sections.  This patch exposed that problem.  With this patch, that gets fixed.
> > > Not sure I follow - the patch description says "Do not implicitly turn on function sections with basic block sections." - which seems in contradiction to the comment here about llc not enabling function sections implicitly with bb sections and this patch fixing it, and this test now having function sections when it didn't before.
> > TLDR; foo requires basic block sections so the entry block also gets a unique section via the newly added function.
> > 
> > Alright, let me explain in more detail.  Previously, llc did not turn on function sections with basic block sections.  So, even though bbsections was enabled, function foo's entry basic block did not get its own section.  That part is clear.  Just for distinction, previously clang did enable function sections implicitly with basic block sections.  So, the same test compiled with clang instead of llc would have seen a unique section for the entry basic block of foo.
> > 
> > Now, with basic block sections enabled, we do not create a separate section for a function that does not need basic block sections, this is with both llc and clang. However, if a function needs basic block sections, like foo in this case, we do create a unique section for its entry basic block too with the newly added function "getUniqueSectionForFunction" in TargetLoweringObjectFileImpl.cpp.
> > 
> > For the counter example, please test basic-block-sections-list.ll.  There, function zip does not need bb sections. So, it will not be placed in a unique function section either and the test explicitly checks for this.
> Oh, this is two different changes in one patch, then. (generally anything with a clang change and an llvm change this is the case, but sometimes they're intrinsically linked/can't be separated (eg: an API change))
> 
> So, step 1) Change LLVM to generate unique sections for functions that need BB sections (that causes this test and others to change) - this has an LLVM effect, and is a feature addition there - but has no effect on the Clang use case, because Clang was/is currently enabling function sections by default when bb sections is used, so the sections don't get any more granular because they're already highly granular
> 
> then (2), once (1) is supported, removes the function-section implication in Clang now that it's not needed to support bb sections (since bb sections can now create its own unique sections on an as-needed basis, rather than needing the shotgun approach of enabling them for all functions)
> 
> Yeah?
> 
> Generally would be helpful if these sort of things were done in separate reviews - I think it'd have made it clear where the behavior is changing (admittedly, if I'd been a bit more clear-headed when reading this, it would've been clear enough that this is changing a bunch of LLVM things, so it's not only a change to Clang's defaults).
> 
> But I think it's reviewed enough here - but please commit it as two separate patches (hopefully that'll help by having more narrow patch descriptions that'll make it clearer what's happening in each step).
Yeah, that works!  I can split it into two patches like you described and add a patch i of 2 to make it clear. Thanks for taking a look!


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

https://reviews.llvm.org/D93876