[PATCH] D118419: [AMDGPU] Allow scalar loads after barrier

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec marked 3 inline comments as done.
rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp:75
 
-bool AMDGPUAnnotateUniformValues::isClobberedInFunction(LoadInst * Load) {
-  const MemoryAccess *MA = MSSA->getWalker()->getClobberingMemoryAccess(Load);
-  return !MSSA->isLiveOnEntryDef(MA);
+bool AMDGPUAnnotateUniformValues::isClobberedInFunction(LoadInst *Load) {
+  MemorySSAWalker *Walker = MSSA->getSkipSelfWalker();
----------------
arsenm wrote:
> This needs a comment explaining why this loop is OK
Will add, but in a nutshell like with any SSA we can go up the tree to the root. Along that route we shall see all defs of the memory location in this case. Provided that phis are carefully procesded, which can create loops in a worst case. What we are looking for in that traversal is a store invalidating scalar cache practically.

Some sort of a confirmation GVN does a similar thing. In fact even the Visited set shall not be needed, we will eventually come to the root and exhaust the list, but that will be a bit slower. A testcase for that is memory_phi tests.

Anyhow, I am running PSDB on this to double check.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp:80
+
+  LLVM_DEBUG(dbgs() << "Checking clobbering of: " << *Load << '\n');
+
----------------
arsenm wrote:
> rampitec wrote:
> > arsenm wrote:
> > > Don't need extra new line
> > It is not extra. This is the actual output on my console:
> > 
> > ```
> > Checking clobbering of:   %i2 = load i32, i32 addrspace(1)* %i1, align 4, !tbaa !15
> >   Def:   fence syncscope("workgroup") acquire
> >   Def:   tail call void @llvm.amdgcn.s.barrier() #3
> >   Def:   fence syncscope("workgroup") release
> >       -> no clobber
> > ```
> Weird that the Instruction and MachineInstr behavior is different
It is.


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

https://reviews.llvm.org/D118419