[PATCH] D130879: [AsmPrinter] Emit PCs into requested PCSections

[Marco Elver via Phabricator via llvm-commits]

melver marked 2 inline comments as done.
melver added a comment.

In D130879#3695546 <https://reviews.llvm.org/D130879#3695546>, @MaskRay wrote:

> I think the patches are overly splitted. I'd merge the 3 patches before `[AsmPrinter] Emit PCs into requested PCSections` into this patch.

See my proposal. I prefer erring on the side of splitting more, especially independent changes as it helps bisectability.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1449
+    OutStreamer->switchSection(S);
+    Prev = Sec;
+  };
----------------
MaskRay wrote:
> What does `Prev = Sec;` do?
Avoid switching the section if it has already been switched from a previous iteration, reducing overall asm output if we have lots of !pcsection'd instructions in the same function.

The comment above mentions that "short-circuiting the common case where the current section is still valid". If the comment could be clearer, let me know.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1490
+  for (const auto &MS : PCSectionsSymbols) {
+    EmitForMD(*MS.first, MS.second, false);
+  }
----------------
MaskRay wrote:
> MaskRay wrote:
> > omit braces for one-line single statements
> As of this patch, no test exercises this line.
> 
> If you add tests to later patches, I think they should be moved here.
It's a chicken-egg problem. We need ISel support for the test to propagate !pcsections through MachineInstrs. So either I add the test here and it'll be broken, or I add it later.

The patch series first adds the new metadata and then AsmPrinter support, and then builds up the plumbing to propagate the metadata. The tests will only start working later.

Based on your suggestion in "[Metadata] Introduce MD_pcsections" I wanted to move the test later. But that seems to contradict your suggestion here.

My proposal is to
- move this patch after "[GlobalISel] Propagate PCSections metadata to MachineInstr" and
- move the test to this patch;
- squash the MCObjectFileInfo changes into this patch;
- keep "[MachineInstr] Allow setting PCSections in ExtraInfo" because it's a dependency for the ISel changes;
- keep "[Metadata] Introduce MD_pcsections" because it's needed for the ISel changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130879