This is an archive of the discontinued LLVM Phabricator instance.

[AsmPrinter] Emit PCs into requested PCSections
ClosedPublic

Authored by melver on Aug 1 2022, 1:56 AM.

Details

Summary

Interpret MD_pcsections in AsmPrinter emitting the requested metadata to
the associated sections. Functions and normal instructions are handled.

Depends on D130886

Diff Detail

Event Timeline

melver created this revision.Aug 1 2022, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 1:56 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
melver requested review of this revision.Aug 1 2022, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 1:56 AM
dvyukov added inline comments.Aug 2 2022, 1:13 AM
llvm/include/llvm/CodeGen/AsmPrinter.h
195

If metadata is used, is the SmallVector actually expected to be small (<=8)?
I assume that SmallVector<8> will allocate space for 8 statically and then waste it if the vector is larger.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1472

Can the other models have pointer size 8?
I assume something like Tiny/Small can have pointer size <4 (?).
Maybe it makes sense to just use getPointerSize() always?

melver marked 2 inline comments as done.Aug 2 2022, 3:33 AM
melver added subscribers: MaskRay, pcc.
melver added inline comments.
llvm/include/llvm/CodeGen/AsmPrinter.h
195

Depends on current usage. If almost all instructions in a functions end up with !pcsections, then it's likely >=8. But currently we're only attaching this to atomics, and I assume the number of atomic ops is low in a function.

But the fact that !pcsections is currently only attached to atomics is something that AsmPrinter really shouldn't care about.

So the choice of 8 is quite arbitrary. SmallVector documentation says to just leave out the small-size, and it'll choose a reasonable default. Let's try that.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1472

The code model doesn't have anything to do with target bitness.

Yes, tiny and small CMs can have 64-bit pointers (small is usually the default).
I don't think we have to worry about 16-bit architectures or 16-bit pointers.

The thing we're optimizing is saving space if we know that the code model doesn't allow addressing (relatively) code or data more than 4 GiB away - even on 64-bit architectures.

The different encoding for different CMs is documented in PCSectionsMetadata.rst.

(The idea to use 32-bit relative relocations was pointed out by @pcc. The fact we should distinguish code models was pointed out by @MaskRay. Adding both as reviewers.)

melver updated this revision to Diff 449249.Aug 2 2022, 3:37 AM
melver marked 2 inline comments as done.

Let SmallVector choose small-size.

MaskRay added inline comments.Aug 2 2022, 8:34 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1458

prefer const reference if non-null

1482

What does Prev = Sec; do?

1519

omit braces for one-line single statements

1523

omit braces for one-line single statements

1523

As of this patch, no test exercises this line.

If you add tests to later patches, I think they should be moved here.

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

melver marked 2 inline comments as done.Aug 3 2022, 8:50 AM

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.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1482

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.

1523

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.
melver updated this revision to Diff 450040.Aug 4 2022, 10:12 AM
melver marked 5 inline comments as done.
melver edited the summary of this revision. (Show Details)

Address comments.

melver updated this revision to Diff 450297.Aug 5 2022, 8:18 AM
  • Change MDNode format to take aux constants as another MDNode.
  • Change createPCSections interface.
melver added a comment.Sep 5 2022, 8:59 AM

PTAL.

@MaskRay - kindly check if I addressed your earlier comments.

Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Sep 7 2022, 2:38 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.