[llvm-commits] atomics memoperands patch

[Mon P Wang]

On Jun 20, 2008, at 12:28 AM, Evan Cheng wrote:

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

Looking at the code, it does seem wrong to add the fields twice.

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

If the CSE can prove that no needed side effects are produced, it can  
do so.  In general though, I would shy away from doing so as it might  
not be easy to prove that is the case.

  -- Mon Ping

> Evan
>