[llvm-commits] atomics memoperands patch
evan.cheng at apple.com
Fri Jun 20 14:26:28 CDT 2008
On Jun 20, 2008, at 9:30 AM, Mon P Wang wrote:
> Hi Evan,
> On Jun 20, 2008, at 12:11 AM, Evan Cheng wrote:
>> 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
> I heard the same comment from Andrew and I think it makes sense. How
> about the following
> ATOMIC_LCS => ATOMIC_CMP_SWAP
> ATOMIC_LAS => ATOMIC_LOAD_ADD
> ATOMIC_LSS => ATOMIC_LOAD_SUB
> This would make the names more consistent.
>> + //! PtrVal - Value of address that we are going to do an atomic
>> 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?
> You are right. We should move what is common up as much as possible.
>> + //! Align - Alignment of memory location in bytes.
>> + unsigned Align;
>> Alignment instead of Align.
> Yep, shows me for getting lazy in typing :->
I am just looking for consistency. :-)
>> + //! 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?
> I think the standard definition is that all these operations are
> volatile and acts like a memory barrier. The atomics should always
> return true for this function for volatile.
Right. So we don't need the field but the isVolatile() predicate
should return true?
>> 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()?
> My assumption when I read the code for LSBaseSDNode that if the
> getBasePtrValue exist, we should use getBasePtrValue because it will
> give more precise information for the location we are accessing (index
> into a location in the FrameIndex). If this is not true, then it is
> not necessary.
Hrm. You are probably right.
>> + return MachineMemOperand(PseudoSourceValue::getFixedStack(),
>> + FI->getIndex(), Size, getAlignment());
>> + else
>> + return MachineMemOperand(getBasePtrValue(), Flags, 0, Size,
>> Also, don't forget to update AddNodeIDNode() and SDNode::dump() in
> I totally forgot about that. Thanks for the reminder.
> -- Mon Ping
>> On Jun 19, 2008, at 6:19 PM, Mon P Wang wrote:
>>> 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.
>>> -- Mon Ping
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
More information about the llvm-commits