[llvm] [DAGCombiner] Don't drop atomic property of original load. (PR #75626)

[Jonas Paulsson via llvm-commits]

JonPsson1 wrote:

> Shouldn't a seq_cst load be an ISD::ATOMIC_LOAD not an ISD::LOAD?

Good question - it seems SystemZ::lowerATOMIC_LOAD() simply replaces the ATOMIC_LOAD with a normal LOAD which is ok generally for the architecture as all naturally aligned loads are atomic.

The MachineMemOperand has the isAtomic() method, which is used by the MemSDNode::isSimple(). LoadSDNode and AtomicSDNode are separate subclasses derived from MemSDNode. This is a little confusing to me: if the isAtomic() is only relevent for AtomicSDNode, AtomicSDNode should override isSimple() and always return 'false', and MemSDNode should not care about isAtomic().

I agree that it could be that the intent is actually that MMO->isAtomic() of any regular LOAD/STORE is ok to ignore, so this patch is not really relevant..? Maybe instead a comment documenting this would be more appropriate if this is the case.

On the other hand, if DAGCombiner is indeed free to ignore the MMO->isAtomic() on any STORE/LOAD, then the SystemZ lowering makes me a little worried. Could DAGCombiner ever e.g. combine loads with an underaligned result, e.g. two 4-byte loads resulting in a single 8-byte load aligned only to 4 bytes?


https://github.com/llvm/llvm-project/pull/75626