[PATCH] D102401: [AMDGPU] Allocate LDS globals in sorted order of their alignment and size.
Mahesha S via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 18 03:35:33 PDT 2021
hsmhsm added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:115
+ SmallVector<const GlobalVariable *, 8> StaticLDSGlobals =
+ collectStaticLDSGlobals(&M);
+
----------------
foad wrote:
> hsmhsm wrote:
> > hsmhsm wrote:
> > > foad wrote:
> > > > hsmhsm wrote:
> > > > > rampitec wrote:
> > > > > > hsmhsm wrote:
> > > > > > > rampitec wrote:
> > > > > > > > Maybe just pass a filter lambda there?
> > > > > > > I personally think that it will be over complicated - collectStaticLDSGlobals() is called by both findVariablesToLower() and getSortedUsedLDSGlobals(). Further filtering of LDS globals is required for findVariablesToLower(), but not for getSortedUsedLDSGlobals().
> > > > > > I do not like the idea of allocating memory, then copying.
> > > > > Then you mean to say, let's return vector, instead of passing it as reference arg? then we cannot use SmallVectorImpl<> as return type, so, we need to go back to returning SmallVector<> itself.
> > > > To avoid allocating memory for a copy of the data, you could filter StaticLDSGlobals in-place with `erase(remove_if(StaticLDSGlobals.begin(), StaticLDSGlobals.end(), predicate), StaticLDSGlobals.end())`.
> > > Probably, we land-up with code like below. We still need to copy to `LocalVars`, because findVariablesToLower() expect return std::vector. And, based on earlier reveiw comments, I have made changes to collectStaticLDSGlobals() to work with SmallVector. Additionally, look at the complexity of the code change in terms of understanding it. So, I thought it is not worth, since we still need to copy anyway.
> > >
> > > ```
> > > StaticLDSGlobals.erase(
> > > std::remove_if(StaticLDSGlobals.begin(), StaticLDSGlobals.end(),
> > > [&](const GlobalVariable *GV) {
> > > GlobalVariable *GV2 = const_cast<GlobalVariable *>(GV);
> > > if (std::none_of(GV2->user_begin(), GV2->user_end(), [&](User *U) {
> > > return userRequiresLowering(UsedList, U);
> > > })) {
> > > return true;
> > > }
> > > return false;
> > > }),
> > > StaticLDSGlobals.end());
> > >
> > > std::vector<llvm::GlobalVariable *> LocalVars(StaticLDSGlobals.begin(),
> > > StaticLDSGlobals.end());
> > > ```
> > Additionally, there is also a added complexity of handling "constness" of "GlobalVariable *"
> ```
> if (std::none_of(GV2->user_begin(), GV2->user_end(), [&](User *U) {
> return userRequiresLowering(UsedList, U);
> })) {
> return true;
> }
> return false;
> ```
> This should just `return std::none_of(...);`
>
> > Additionally, there is also a added complexity of handling "constness" of "GlobalVariable *"
>
> Then fix `userRequiresLowering` first? Something like: https://reviews.llvm.org/differential/diff/346095/
>> This should just return std::none_of(...);
Agree
>> Then fix userRequiresLowering first? Something like: https://reviews.llvm.org/differential/diff/346095/
I assume, it is not as simple as that, and I guess, it will force me to change within `AMDGPULowerModuleLDSPass.cpp` which I want to avoid at this point. These kind of changes are better to implement as a kind of code refactor patch. But, nevetheless, if you guys think that it is must, then, I will do it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102401/new/
https://reviews.llvm.org/D102401
More information about the llvm-commits
mailing list