[PATCH] D109870: [AMDGPU] Enable the pass "amdgpu-replace-lds-use-with-pointer"

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:199
+    auto *EI = &(*(EBB.getFirstInsertionPt()));
+    BasicBlock::reverse_iterator RIT(EBB.getTerminator());
+    while (!isa<AllocaInst>(*RIT) && (&*RIT != EI))
----------------
hsmhsm wrote:
> arsenm wrote:
> > hsmhsm wrote:
> > > hsmhsm wrote:
> > > > arsenm wrote:
> > > > > Why is this using a reverse iterator? Why not just forward iterate since that's where the allocas are?
> > > > (At least in the failed openmp test case), what I noticed is that alloca at the beginning are not contiguous one after other, but there are in between addressspace casts like below (I am not sure, if there is a possibility of other in-between instructions too other than addressspace). So I went with the reverse iteration. But, actually if we are able to successfully forward iterate, that would very neat.
> > > > 
> > > > alloca
> > > > addresss-space-cast 
> > > > alloca
> > > > addresss-space-cast 
> > > > ....
> > > Another possibility is as below:
> > > 
> > > Since we only care about allocas inserted at the beginning of the block, and since we need to split the block before any call (since called functions is where the initialized pointers are accessed), and since it is safe to assume that all entry point allocas do appear before any call, we can forward traverse until first call, and split the block at that point.
> > Can we fix the insertion of these allocas to not intersperse addrspacecasts?
> Let me check if we can handle it.
I though the idea was to fix the alloca insertion, not to move them. Even if we want to cluster them it does not seem to be a right place to do it here as it is a more general thing. Really I'd prefer to fix FE and produce a canonical IR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109870