[llvm-commits] atomics memoperands patch

[Evan Cheng]

Thanks Mon Ping. Some comments:

+           N->getOpcode() == ISD::ATOMIC_LCS  ||
+           N->getOpcode() == ISD::ATOMIC_LAS  ||
+           N->getOpcode() == ISD::ATOMIC_SWAP ||
+           N->getOpcode() == ISD::ATOMIC_LSS  ||
+           N->getOpcode() == ISD::ATOMIC_LOAD_AND  ||
+           N->getOpcode() == ISD::ATOMIC_LOAD_OR   ||
+           N->getOpcode() == ISD::ATOMIC_LOAD_XOR  ||

Does it make sense to take this opportunity to rename lcs, las, and lss?

+  //! PtrVal - Value of address that we are going to do an atomic  
update
How about "Value of atomically updated address"?

+  const Value* PtrVal;

Since this is basically the same as LSBaseSDNode SrcValue, how about  
move it to the common base class MemSDNode?

+
+  //! Align - Alignment of memory location in bytes.
+  unsigned Align;

Alignment instead of Align.

+
+  //! IsVol - True if the store is volatile.
+  bool IsVol;

IsVolatile instead of IsVol. But do we need these? Isn't it always  
volatile given it's an atomic operation?

Again, LSBaseSDNode has these fields as well. Perhaps move them up?

+  // Check if the atomic references a frame index
+  const FrameIndexSDNode *FI =
+    dyn_cast<const FrameIndexSDNode>(getBasePtr().Val);
+  if (!getBasePtrValue() && FI)

I am not quite sure why it's necessary to check getBasePtrValue()?


+    return MachineMemOperand(PseudoSourceValue::getFixedStack(), Flags,
+                             FI->getIndex(), Size, getAlignment());
+  else
+    return MachineMemOperand(getBasePtrValue(), Flags, 0, Size,  
getAlignment());
+}

Also, don't forget to update AddNodeIDNode() and SDNode::dump() in  
SelectionDAG.cpp.

Thanks,

Evan

On Jun 19, 2008, at 6:19 PM, Mon P Wang wrote:

> Hi,
>
> Like loads/stores, atomics touches memory and should have an  
> associated MemOperand with the operation.  The following patch  
> creates an abstract class MemSDNode that all nodes  which have an  
> associated MemOperand need to inherit to.  It also creates a new  
> property SDNPMemOperand that indicates that a node touches memory  
> and has an associated MemOperand.If you have any comments, please  
> let me know.
>
> Thanks,
>  -- Mon Ping
>
> <memop.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits