[llvm-commits] atomics memoperands patch

[Mon P Wang]

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

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 :->

> +
> +  //! 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.

> 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.

>
> +    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