[llvm-commits] atomics memoperands patch

[Mon P Wang]

That right. The field will be removed and the function on the atomic  
will always return true.

-- Mon Ping

On Jun 20, 2008, at 12:26 PM, Evan Cheng wrote:

> [Deleted Text]
>>
>>> +
>>> +  //! 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits