[ARM] Be super conservative about atomics

[reames]

Summary

As requested during review of D57601, be equally conservative for atomic MMOs as for volatile MMOs in all in tree backends. At the moment, all atomic MMOs are also volatile, but I'm about to change that.

I don't really like this patch since I can't test it. I have no idea what "correct" codegen looks like for these targets, so I'm taking "no tests fail" to mean the patch is correct. If any target owner wants to take over their part and write better tests, please feel free.

[uweigand]

The SystemZ part LGTM, thanks.

[kparzysz]

The Hexagon changes are OK.

[arsenm]

Should all of these use hasOrderedMemoryRef instead?

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

1583–1584 ↗

(On Diff #187722)

Is this really any different than hasOrderedMemoryRef?

[reames]

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

1583–1584 ↗

(On Diff #187722)

They're not obviously equivalent. I have no idea if they'd achieve the same effect here. If someone more knowledgeable of the code wants to make a deeper change, feel free.

[reames]

Rebase after landing SystemZ and Hexagon portions which had been previously LGTMed.

ping to ARM and Lanai maintainers

[chill]

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

1584 ↗

(On Diff #188065)

How about using MachineMemOperand::isUnordered() instead?

[john.brawn]

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

1584 ↗

(On Diff #188065)

While it's probably valid to combine multiple unordered loads/stores into an ldm/stm (and also monotonic ones as well I think) the atomicity gets lost and the ldm/stm is not marked as atomic. I don't know if that actually leads to any problems, but I think we should be safe and just reject all atomic instructions for now. Maybe the TODO should be changed to something like

// TODO: We could allow unordered and monotonic atomics here, but we need to make sure the resulting ldm/stm is correctly marked as atomic.

[jpienaar]

Change to Lanai seems fine. The model for Lanai execution/usage means atomics aren't important, but treating them conservatively is better. Thanks!

[reames]

Rebase after landing Lanai and addressing ARM comment. Pending LGTM from ARM contributors.

[john.brawn]

LGTM.

llvm/trunk/lib/Target/ARM/ARMLoadStoreOptimizer.cpp

static bool isMemoryOp(const MachineInstr &MI) {
   // When no memory operands are present, conservatively assume unaligned,
   // volatile, unfoldable.
   if (!MI.hasOneMemOperand())
     return false;
 
   const MachineMemOperand &MMO = **MI.memoperands_begin();
 
   // Don't touch volatile memory accesses - we may be changing their order.
-  if (MMO.isVolatile())
+  // TODO: We could allow unordered and monotonic atomics here, but we need to
+  // make sure the resulting ldm/stm is correctly marked as atomic. 
+  if (MMO.isVolatile() || MMO.isAtomic())
     return false;
 
   // Unaligned ldr/str is emulated by some kernels, but unaligned ldm/stm is
   // not.
   if (MMO.getAlignment() < 4)
     return false;
 
   // str <undef> could probably be eliminated entirely, but for now we just want
ARMPreAllocLoadStoreOpt::CanFormLdStDWord(MachineInstr *Op0, MachineInstr *Op1,
   } else {
     return false;
   }
 
   // Make sure the base address satisfies i64 ld / st alignment requirement.
   // At the moment, we ignore the memoryoperand's value.
   // If we want to use AliasAnalysis, we should check it accordingly.
   if (!Op0->hasOneMemOperand() ||
-      (*Op0->memoperands_begin())->isVolatile())
+      (*Op0->memoperands_begin())->isVolatile() ||
+      (*Op0->memoperands_begin())->isAtomic())
     return false;
 
   unsigned Align = (*Op0->memoperands_begin())->getAlignment();
   const Function &Func = MF->getFunction();
   unsigned ReqAlign = STI->hasV6Ops()
     ? TD->getABITypeAlignment(Type::getInt64Ty(Func.getContext()))
     : 8;  // Pre-v6 need 8-byte align
   if (Align < ReqAlign)