[PATCH] D152833: A new code layout algorithm for function reordering [1/3]
Hongtao Yu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 5 15:30:30 PDT 2023
hoy added inline comments.
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:161
+/// together with the corresponding merge 'type' and 'offset'.
+struct MergeGainT {
+ explicit MergeGainT() = default;
----------------
spupyrev wrote:
> hoy wrote:
> > nit: by convention the `T` in the struct name is not needed. I'm seeing `JumpT` and a few other types getting `T`. Is there a particular reason?
> I too don't like this T suffix but I found myself writing
> ```
> for (Chain *Chain : SortedChains)
> ```
> all over the place. It compiles but looks confusing. I strongly prefer `ChainT *Chain` over this instead.
>
> Any other suggestions?
I usually do
```
for (auto *Chain : SortedChains)
```
or
```
for (Chain *C : SortedChains)
```
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:991
+std::vector<uint64_t>
+llvm::applyExtTspLayout(const std::vector<uint64_t> &NodeSizes,
+ const std::vector<uint64_t> &NodeCounts,
----------------
spupyrev wrote:
> hoy wrote:
> > I guess CDS is only for function layout, but do you think the name here should be adjusted in order to not be Exstsp-specific?
> Can you clarify what "name" you refer to? (happy to rename or add a comment if there is a confusion)
I mean `applyExtTspLayout`. Do you think it makes sense to rename it like `applyCDSLayout`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152833/new/
https://reviews.llvm.org/D152833
More information about the llvm-commits
mailing list