[PATCH] D101177: [AMDGPU] Avoid adding nullptr keys to hash table

[Piotr Sobczak via Phabricator via llvm-commits]

piotr added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:1489-1490
         const Value *Ptr = Memop->getValue();
-        SLoadAddresses.insert(std::make_pair(Ptr, Inst.getParent()));
+        if (Ptr)
+          SLoadAddresses.insert(std::make_pair(Ptr, Inst.getParent()));
       }
----------------
foad wrote:
> If we don't do anything here when !Ptr, then we have lost the information that there is an outstanding scalar load, which does not seem safe to me.
> 
> Stepping back a bit, I don't think the original SLoadAddresses implementation in D71934 was safe. Surely a scalar load and a vector store can alias even if they don't have an identical Ptr value?
Agreed that my change is not correct either.

Looks D71934 was a best effort fix and did not intend to be a final one ("Proper fix should include creating the alias analysis on the machine IR that is obviously too big hummer at the moment.").

The reason I started looking at this is that the map gets corrupted if there are multiple elements with the same key and different values.

Possibly, I should be looking at places where the memop data is dropped and fix that at source.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101177