[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

[Eli Friedman]

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.
>
> Modified:
>     llvm/trunk/include/llvm/CodeGen/SelectionDAGNodes.h
>     llvm/trunk/include/llvm/Target/TargetSelectionDAG.td
>     llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>     llvm/trunk/test/CodeGen/Hexagon/newvaluejump.ll
>     llvm/trunk/test/CodeGen/Hexagon/newvaluestore.ll
>
> Modified: llvm/trunk/include/llvm/CodeGen/SelectionDAGNodes.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/SelectionDAGNodes.h?rev=162733&r1=162732&r2=162733&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/SelectionDAGNodes.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/SelectionDAGNodes.h Mon Aug 27 22:11:32 2012
> @@ -1011,11 +1011,6 @@
>      SubclassData |= SynchScope << 12;
>      assert(getOrdering() == Ordering && "Ordering encoding error!");
>      assert(getSynchScope() == SynchScope && "Synch-scope encoding error!");
> -
> -    assert((readMem() || getOrdering() <= Monotonic) &&
> -           "Acquire/Release MachineMemOperand must be a load!");
> -    assert((writeMem() || getOrdering() <= Monotonic) &&
> -           "Acquire/Release MachineMemOperand must be a store!");
>    }
>
>  public:
>
> Modified: llvm/trunk/include/llvm/Target/TargetSelectionDAG.td
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Target/TargetSelectionDAG.td?rev=162733&r1=162732&r2=162733&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Target/TargetSelectionDAG.td (original)
> +++ llvm/trunk/include/llvm/Target/TargetSelectionDAG.td Mon Aug 27 22:11:32 2012
> @@ -445,9 +445,9 @@
>  def atomic_load_umax : SDNode<"ISD::ATOMIC_LOAD_UMAX", SDTAtomic2,
>                      [SDNPHasChain, SDNPMayStore, SDNPMayLoad, SDNPMemOperand]>;
>  def atomic_load      : SDNode<"ISD::ATOMIC_LOAD", SDTAtomicLoad,
> -                    [SDNPHasChain, SDNPMayStore, SDNPMayLoad, SDNPMemOperand]>;
> +                    [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
>  def atomic_store     : SDNode<"ISD::ATOMIC_STORE", SDTAtomicStore,
> -                    [SDNPHasChain, SDNPMayStore, SDNPMayLoad, SDNPMemOperand]>;
> +                    [SDNPHasChain, SDNPMayStore, SDNPMemOperand]>;
>
>  // Do not use ld, st directly. Use load, extload, sextload, zextload, store,
>  // and truncst (see below).
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp?rev=162733&r1=162732&r2=162733&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Mon Aug 27 22:11:32 2012
> @@ -3927,12 +3927,16 @@
>      Alignment = getEVTAlignment(MemVT);
>
>    MachineFunction &MF = getMachineFunction();
> -  unsigned Flags = MachineMemOperand::MOLoad | MachineMemOperand::MOStore;
>
> +  // All atomics are load and store, except for ATMOIC_LOAD and ATOMIC_STORE.
>    // For now, atomics are considered to be volatile always.
>    // FIXME: Volatile isn't really correct; we should keep track of atomic
>    // orderings in the memoperand.
> -  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.

-Eli