[PATCH] D102401: [AMDGPU] Allocate LDS globals in sorted order of their alignment and size.

[Mahesha S via Phabricator via llvm-commits]

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:
> > 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());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102401