[PATCH] D152706: [AMDGPU] Use SSAUpdater in PromoteAlloca

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp:341-358
+  bool HasSeenBB = false;
+  BasicBlock *BB = I->getParent();
+  while (InsertIt != WorkList.end()) {
+    // -> Instruction is in the same basic block. If the new instruction comes
+    // before it, insert here.
+    //
+    // -> Instruction is in a different basic block: If we've already seen the
----------------
arsenm wrote:
> Pierre-vh wrote:
> > arsenm wrote:
> > > This whole thing feels wrong to me. You shouldn't need to figure out any ordering, much less by using comesBefore repeatedly
> > I think that the issue was introduced because I'm looking through things such as bitcasts, which means that we can have IR like this:
> > ```
> > bb1:
> >   %x = alloca ...
> >   %cast.x = cast %x to ...
> > 
> > bb2: 
> >   store ... %x
> >   store ... %cast.x
> > ```
> > 
> > Then, when I iterate the users of the alloca and build the worklist, the worklist may have the store to %cast.x before the store to %x, so I need to sort the worklist to use SSAAUpdater.
> > 
> > I'm not sure how `LoadStorePromoter` would help, I've looked at it but it seems to be for a different purpose
> I applied your patch and not that many tests fail. The one I'm looking at is promote_memcpy_inline_aggr. This has the multiple loads and stores in the same block case the LoadAndStorePromoter comment says the base SSAUpdater does not handle. I think this is probably the correct tool, you just have the added complexity that we're coercing the type of the accesses to vector instead of reusing the original 
`LoadAndStorePromoter` would need some modifications to work here or I would need to first rewrite all of the load/stores to use vectors then run promote alloca later to do the promotion. For instance, `LoadAndStorePromoter` initializes SSAUpdater with the type of the first load/store

I don't mind using it if you think it's better to first rewrite all the load/stores then run `LoadAndStoreUpdater`, but IMO it's really not necessary, we'd make things more complex (and less direct) just to avoid a 5 lines function (I heavily simplified it)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152706