[PATCH] D88517: [BasicBlockSections] Make sure that the labels for address-taken blocks are emitted after switching the seciton.

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl created this revision.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.
rahmanl retitled this revision from "Emit the label of address-taken blocks within their assigned section." to "Switch section before emitting the labels for address-taken blocks.".
rahmanl edited the summary of this revision.
rahmanl updated this revision to Diff 295180.
rahmanl marked 2 inline comments as done.
rahmanl added a comment.
rahmanl retitled this revision from "Switch section before emitting the labels for address-taken blocks." to "[BasicBlockSections] Make sure that the labels for address-taken blocks are emitted after switching the seciton.".
rahmanl edited the summary of this revision.
rahmanl added reviewers: efriedma, tmsriram, snehasish, MaskRay.
rahmanl published this revision for review.

- Change the test to -basic-block-sections.


rahmanl added a comment.

Thanks for the comments @tmsriram and @snehasish.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3089
 
+  // Emit label of the basic block if needed.
   if (MBB.pred_empty() ||
----------------
I would like a comment which explains the condition but this comment isn't very helpful. Consider rewording or removing.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3089
 
+  // Emit label of the basic block if needed.
   if (MBB.pred_empty() ||
----------------
snehasish wrote:
> I would like a comment which explains the condition but this comment isn't very helpful. Consider rewording or removing.
Thanks for the comment. If you look at D73674, this exact comment was initially there but we removed it because we wanted the same if-else block to serve other purposes as well. This patch kinds of reverts that change.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3099
-    }
-    OutStreamer->emitLabel(BBSymbol);
-    // With BB sections, each basic block must handle CFI information on its own
----------------
Could you elaborate on what you mean by label is emitted prior to switching the section?  Here it shows as label is emitted only after switching the section.  Maybe a simple example would help.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3099
-    }
-    OutStreamer->emitLabel(BBSymbol);
-    // With BB sections, each basic block must handle CFI information on its own
----------------
tmsriram wrote:
> Could you elaborate on what you mean by label is emitted prior to switching the section?  Here it shows as label is emitted only after switching the section.  Maybe a simple example would help.
It's actually about the ".Ltmp" label emitted in the "MBB.hasAddressTaken()" if block. This patch moves the section switching code before that block. One thing I can suggest is to run the included test on trunk and see the failure.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3107
+  // AsmPrinterHandler::beginFunction).
+  if (MBB.isBeginSection() && !MBB.pred_empty())
+    for (const HandlerInfo &HI : Handlers)
----------------
Wasn't there some concern about empty blocks being present which might confound this check? 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3107
+  // AsmPrinterHandler::beginFunction).
+  if (MBB.isBeginSection() && !MBB.pred_empty())
+    for (const HandlerInfo &HI : Handlers)
----------------
snehasish wrote:
> Wasn't there some concern about empty blocks being present which might confound this check? 
It is OK for empty basic blocks to begin a section.


================
Comment at: llvm/test/CodeGen/X86/machine-function-splitter-blockaddress.ll:3
+
+define void @foo1(i1 zeroext %0) nounwind !prof !0 !section_prefix !1 {
+; CHECK:         .text
----------------
I think this should be part of basic block sections. MFS is using basic block sections and this addresses the underlying issue.



================
Comment at: llvm/test/CodeGen/X86/machine-function-splitter-blockaddress.ll:3
+
+define void @foo1(i1 zeroext %0) nounwind !prof !0 !section_prefix !1 {
+; CHECK:         .text
----------------
snehasish wrote:
> I think this should be part of basic block sections. MFS is using basic block sections and this addresses the underlying issue.
> 
Yes. Changed to a basic-block-sections test.


Currently, AsmPrinter code is organized in a way in which the labels of address-taken blocks are emitted in the previous section, which makes the relocation incorrect.
This patch reorganizes the code to switch to the basic block section before handling address-taken blocks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88517

Files:
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/test/CodeGen/X86/basic-block-sections-blockaddress.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D88517.295180.patch
Type: text/x-patch
Size: 3972 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200930/fc2b0fee/attachment.bin>