[llvm-commits] [llvm] r162733 - in /llvm/trunk: include/llvm/CodeGen/SelectionDAGNodes.h include/llvm/Target/TargetSelectionDAG.td lib/CodeGen/SelectionDAG/SelectionDAG.cpp test/CodeGen/Hexagon/newvaluejump.ll test/CodeGen/Hexagon/newvaluestore.l

[Jakob Stoklund Olesen]

On Aug 29, 2012, at 11:45 AM, Eli Friedman <eli.friedman at gmail.com> wrote:

> On Mon, Aug 27, 2012 at 8:11 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
>> Author: stoklund
>> Date: Mon Aug 27 22:11:32 2012
>> New Revision: 162733
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=162733&view=rev
>> Log:
>> Remove extra MayLoad/MayStore flags from atomic_load/store.
>> 
>> These extra flags are not required to properly order the atomic
>> load/store instructions. SelectionDAGBuilder chains atomics as if they
>> were volatile, and SelectionDAG::getAtomic() sets the isVolatile bit on
>> the memory operands of all atomic operations.
>> 
>> The volatile bit is enough to order atomic loads and stores during and
>> after SelectionDAG.
>> 
>> This means we set mayLoad on atomic_load, mayStore on atomic_store, and
>> mayLoad+mayStore on the remaining atomic read-modify-write operations.
>> -  Flags |= MachineMemOperand::MOVolatile;
>> +  unsigned Flags = MachineMemOperand::MOVolatile;
>> +  if (Opcode != ISD::ATOMIC_STORE)
>> +    Flags |= MachineMemOperand::MOLoad;
>> +  if (Opcode != ISD::ATOMIC_LOAD)
>> +    Flags |= MachineMemOperand::MOStore;
> 
> I'm not sure this is safe; I suppose it depends on how exactly
> "volatile" is defined, though.  Specifically, it's unsafe to move an
> arbitrary load from after an "acquire" load to before it.

Right.

I believe that technically, volatile loads are only required to be ordered with respect to each other. We treat them as full scheduling barriers in the code generator, though.

It seems to be possible for an MI pass using MachineInstr::isSafeToMove() to move a normal load across an atomic load at the moment. We should fix that.

/jakob