[PATCH] D102096: [DAGCombiner] Fix DAG combine store elimination, different address space.

Tue May 11 11:20:56 PDT 2021

hgreving added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17919
         ST->isUnindexed() && ST->isSimple() &&
+        Ld->getAddressSpace() == ST->getAddressSpace() &&
         // There can't be any side effects between the load and store, such as
----------------
jrtc27 wrote:
> hgreving wrote:
> > jrtc27 wrote:
> > > Should this be weakened using `isNoopAddrSpaceCast` both(?) ways round?
> > I am actually not sure about the _exact_ semantics of isNoopAddrSpaceCast(). Does it imply that if true, it is a complete nop w.r.t. the memory model?
> memcpy/memmove/memset libcall lowering at least relies on being able to lower calls with any pointers that can be no-op cast to address space 0 (see checkAddrSpaceIsValidForLibcall). CodeGenPrepare also peeks through them though haven't looked exactly how.
My take on this would be to stay rather slightly conservative and fix this bug, which can lead to clear miscompiles, unless somebody jumps in and confirms the exact semantics of isNoopAddrSpaceCast(). The comment just says "is a nop". This doesn't directly mean to me that one can eliminate a store like that. Then OTOH, if the two stores are in any way different, then the cast should not be a nop either, so maybe yes?

Should I add a TODO comment here maybe?

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

https://reviews.llvm.org/D102096