Page MenuHomePhabricator

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

Authored by rahmanl on Sep 29 2020, 3:17 PM.

Details

Summary

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.

Diff Detail

Event Timeline

rahmanl created this revision.Sep 29 2020, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2020, 3:17 PM
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..Sep 29 2020, 3:19 PM
rahmanl edited the summary of this revision. (Show Details)
snehasish added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3089

I would like a comment which explains the condition but this comment isn't very helpful. Consider rewording or removing.

3107

Wasn't there some concern about empty blocks being present which might confound this check?

llvm/test/CodeGen/X86/machine-function-splitter-blockaddress.ll
3

I think this should be part of basic block sections. MFS is using basic block sections and this addresses the underlying issue.

tmsriram added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3099

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.

rahmanl updated this revision to Diff 295180.Sep 29 2020, 11:41 PM
rahmanl marked 2 inline comments as done.
  • Change the test to -basic-block-sections.

Thanks for the comments @tmsriram and @snehasish.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3089

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.

3099

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.

3107

It is OK for empty basic blocks to begin a section.

llvm/test/CodeGen/X86/machine-function-splitter-blockaddress.ll
3

Yes. Changed to a basic-block-sections test.

rahmanl published this revision for review.Sep 30 2020, 11:51 AM
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. (Show Details)
snehasish accepted this revision.Oct 6 2020, 3:29 PM

lgtm with some minor comments.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3051

Can you extend the comment here to state that this needs to happen before the if (MBB.hasAddressTaken()) part below?

llvm/test/CodeGen/X86/basic-block-sections-blockaddress.ll
1 ↗(On Diff #295180)

nit: perhaps a better name for this file is "basic-block-sections-address-taken.ll"

28 ↗(On Diff #295180)

I think you meant to only indent the instructions, the labels should be indented less here and above?

This revision is now accepted and ready to land.Oct 6 2020, 3:29 PM
MaskRay accepted this revision.Oct 6 2020, 4:41 PM
MaskRay added inline comments.
llvm/test/CodeGen/X86/basic-block-sections-blockaddress.ll
28 ↗(On Diff #295180)

The excess indentation is here to satisfy the longest directive CHECK-LABEL.

snehasish added inline comments.Oct 6 2020, 4:59 PM
llvm/test/CodeGen/X86/basic-block-sections-blockaddress.ll
28 ↗(On Diff #295180)

Interesting, on my end I see that the indentation lines it up with the instructions below. Is this a phabricator quirk?
https://paste.pics/ABM6T

rahmanl updated this revision to Diff 296780.Oct 7 2020, 1:18 PM
rahmanl marked 4 inline comments as done.
  • Rename test, and fix indentation.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3051

Thanks for the comment, but I am not sure if it's necessary. (Likewise the alignment code above must be before switching the section, but we don't explicitly mention that).

llvm/test/CodeGen/X86/basic-block-sections-blockaddress.ll
1 ↗(On Diff #295180)

Changed the test name to basic-block-sections-blockaddress-taken.ll.

28 ↗(On Diff #295180)

The indentation for .Ltmp1: was incorrect. I fixed it.

This revision was landed with ongoing or failed builds.Oct 7 2020, 1:22 PM
This revision was automatically updated to reflect the committed changes.