This is an archive of the discontinued LLVM Phabricator instance.

[CodeLayout] Refactor std::vector uses, namespace, and EdgeCountT. NFC
ClosedPublic

Authored by MaskRay on Sep 17 2023, 2:39 PM.

Details

Summary
  • Place types and functions in the llvm::codelayout namespace
  • Change EdgeCountT from pair<pair<uint64_t, uint64_t>, uint64_t> to a struct and utilize structured bindings. It is not conventional to use the "T" suffix for structure types.
  • Remove a redundant copy in ChainT::merge.
  • Change {ExtTSPImpl,CDSortImpl}::run to use return value instead of an output parameter
  • Rename applyCDSLayout to computeCacheDirectedLayout: (a) avoid rare abbreviation "CDS" (cache-directed sort) (b) "compute" is more conventional for the specific use case
  • Change the parameter types from std::vector to ArrayRef so that SmallVector arguments can be used.
  • Similarly, rename applyExtTspLayout to computeExtTspLayout.

Diff Detail

Event Timeline

MaskRay created this revision.Sep 17 2023, 2:39 PM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay requested review of this revision.Sep 17 2023, 2:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2023, 2:39 PM
MaskRay updated this revision to Diff 556919.Sep 17 2023, 4:31 PM
MaskRay retitled this revision from [CodeLayout] Refactor some std::vector uses. NFC to [CodeLayout] Refactor some std::vector and nested std::pair uses. NFC.
MaskRay edited the summary of this revision. (Show Details)

.

MaskRay updated this revision to Diff 556920.Sep 17 2023, 4:38 PM

remove JumpT

dblaikie added inline comments.
llvm/include/llvm/Transforms/Utils/CodeLayout.h
26–31

Might be at the point where this should/could be a struct with named members?

llvm/lib/Transforms/Utils/CodeLayout.cpp
1038

Then this (& elsewhere) wouldn't need a structured binding - it could access the members by name directly?

Isn't phabricator deprecated now, and all new reviews should be done on github?

MaskRay added a comment.EditedSep 18 2023, 12:46 PM

Isn't phabricator deprecated now, and all new reviews should be done on github?

New reviews will be disabled on Oct 1 (or later, as I may be the operator and I will be out of town...). For this change it is convenient to use Phabricator to link to D152840

MaskRay updated this revision to Diff 556973.Sep 18 2023, 2:20 PM
MaskRay retitled this revision from [CodeLayout] Refactor some std::vector and nested std::pair uses. NFC to [CodeLayout] Refactor std::vector uses, namespace, and EdgeCountT. NFC.
MaskRay edited the summary of this revision. (Show Details)

place functions under llvm::codelayout::

change EdgeCountT to a struct named EdgeCount

dblaikie added inline comments.Sep 18 2023, 3:31 PM
llvm/lib/Transforms/Utils/CodeLayout.cpp
1402–1404

I probably wouldn't use a structured binding if it's a struct with named members already (obfuscates that fact a bit, risks naming the bindings differently than the members, or having them otherwise get out of sync, etc). (similarly elsewhere in this change)

MaskRay updated this revision to Diff 556981.Sep 18 2023, 4:19 PM

avoid structured bindings

Amir added a comment.Sep 21 2023, 11:07 AM

Thank you for the refactoring. Looks good, but let me retrigger testing with the latest changes.
ping @spupyrev

Amir accepted this revision.Sep 21 2023, 12:31 PM
This revision is now accepted and ready to land.Sep 21 2023, 12:31 PM

Thank you for the stamp. I'll land this soon so that D152840 can rebase on top of it and we can have D152840 soon...