[llvm-commits] atomics memoperands patch

[Evan Cheng]

Questions for everyone. I've just noticed something in SelectionDAG.cpp:

SDNode *SelectionDAG::FindModifiedNodeSlot(SDNode *N,
                                            SDOperandPtr Ops,unsigned  
NumOps,
                                            void *&InsertPos) {
...

   FoldingSetNodeID ID;
   AddNodeIDNode(ID, N->getOpcode(), N->getVTList(), Ops, NumOps);

   if (const LoadSDNode *LD = dyn_cast<LoadSDNode>(N)) {
     ID.AddInteger(LD->getAddressingMode());
     ID.AddInteger(LD->getExtensionType());
     ID.AddInteger(LD->getMemoryVT().getRawBits());
     ID.AddInteger(LD->getAlignment());
     ID.AddInteger(LD->isVolatile());
   } else if (const StoreSDNode *ST = dyn_cast<StoreSDNode>(N)) {
     ID.AddInteger(ST->getAddressingMode());
     ID.AddInteger(ST->isTruncatingStore());
     ID.AddInteger(ST->getMemoryVT().getRawBits());
     ID.AddInteger(ST->getAlignment());
     ID.AddInteger(ST->isVolatile());
   }

Do we need the if () else if? AddNodeIDNode has already added those  
fields to the FoldingSetNodeId.

Also, do we want to cse nodes that are volatile?

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