[llvm-commits] atomics memoperands patch

[Evan Cheng]

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
>> lss?
>>
>
> 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

Yes!

>
>
> This would make the names more consistent.
>
>> +  //! 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?
>
> 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.

Evan
>
>
>>
>> +    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.
>
>
> I totally forgot about that.  Thanks for the reminder.
>
>
>   -- Mon Ping
>
>> 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
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits