[llvm-commits] atomic operator patch

[Mon P Wang]

Hi Evan,

I'll mark all the atomic operators that I added to expand.  I wasn't  
planning on it adding the others syncs yet.

Thanks,
   -- Mon Ping

On Apr 30, 2008, at 9:29 AM, Evan Cheng wrote:

> More comments:
>
>
> +SDOperand X86TargetLowering::LowerLSS(SDOperand Op, SelectionDAG  
> &DAG) {
> +  return SDOperand(ExpandATOMIC_LSS(Op.Val, DAG), 0);
> +}
> +
> +SDNode* X86TargetLowering::ExpandATOMIC_LSS(SDNode* Op,  
> SelectionDAG &DAG) {
> +  MVT::ValueType T = cast<AtomicSDNode>(Op)->getVT();
> +  assert (T == MVT::i32 && "Only know how to expand i32 LSS");
> +  SDOperand negOp = DAG.getNode(ISD::SUB, T,
> +                                DAG.getConstant(0, T), Op- 
> >getOperand(2));
> +  return DAG.getAtomic(ISD::ATOMIC_LAS, Op->getOperand(0),
> +                       Op->getOperand(1), negOp, T).Val;
> +}
>
> It seems silly to have a LowerLSS which just call ExpandATOMiC_LSS.  
> Please just mark ATOMIC_LSS as "Exapnd" instead of "Custom".
>
> Also, are you going to implement __sync_add_and_fetch, etc.? If so,  
> I've attached a (not fully tested) patch for X86InstrInfo.td. You  
> can incorporate it if you wish.
>
> Evan
> <op.atomic-add>
>
>
> On Apr 30, 2008, at 8:31 AM, Dan Gohman wrote:
>
>> Hi Mon Ping,
>>
>> Thanks for working on the atomic builtins. Below are some
>> review comments.
>>
>> +// Expand atomics bitwise operation
>> +//  Op: node to expand,
>> +//  BitOpc: ISD::AND, ISD::OR, ISD::XOR
>> +SDNode* X86TargetLowering::ExpandAtomicBitwise(SDNode* Op,  
>> SelectionDAG
>> &DAG,
>> +                                               ISD::NodeType  
>> BitOpc) {
>> +  MVT::ValueType T = cast<AtomicSDNode>(Op)->getVT();
>> +  assert (T == MVT::i32 && "Only know how to expand i32 Bitwise  
>> op");
>> +  SDOperand ldOp = DAG.getLoad(T, Op->getOperand(0), Op- 
>> >getOperand(1),
>> +                               NULL, 0 , false);
>> +  SDOperand binOp = DAG.getNode(BitOpc, T, ldOp, Op->getOperand(2));
>> +  return DAG.getAtomic(ISD::ATOMIC_SWAP, Op->getOperand(0),
>> Op->getOperand(1),
>> +                       binOp, T).Val;
>> +
>> +}
>>
>> This doesn't look atomic. Between the load and the swap another
>> thread could modify the memory.
>>
>> +                       GCCBuiltin<"__sync_fetch_and_min">;
>> +                       GCCBuiltin<"__sync_fetch_and_max">;
>> +                       GCCBuiltin<"__sync_fetch_and_minu">;
>> +                       GCCBuiltin<"__sync_fetch_and_maxu">;
>>
>> I didn't see these in the GCC 4.3 manual; are these new names?
>> If so, can I suggest umin/umax (and maybe even smin/smax for
>> the signed ones) for the names? It would be more consistent
>> with other similar names in llvm (sdiv/udiv, slt/ult, etc.)
>>
>> +  // Defines the capacity of the TargetLowering::OpActions table
>> +#define OpActionsCapacity 173
>>
>> Could you make this a static const, or an enum? As a macro, it
>> doesn't live in the llvm namespace.
>>
>> +    ATOMIC_LSS,
>>
>> +    ATOMIC_LANDS,
>> +    ATOMIC_LORS,
>> +    ATOMIC_LXORS,
>> +    ATOMIC_LMINS,
>> +    ATOMIC_LMAXS,
>> +    ATOMIC_LMINUS,
>> +    ATOMIC_LMAXUS,
>>
>> This L<op>S naming convention is getting a bit hairy. LMINUS in
>> particular has the misfortune of looking like the word "minus".
>> How about
>> ATOMIC_LOAD_AND
>> ATOMIC_LOAD_OR
>> and so on? I know this is different from the existing atomics in
>> LLVM, but I wouldn't object to renaming those too :-).
>>
>> Thanks,
>>
>> Dan
>>
>> On Tue, April 29, 2008 8:39 pm, Mon P Wang wrote:
>>>
>>> I have updated the patch because I forgot to delete some obsolete
>>> functions as well as removed some unnecessary indentation changes.
>>>
>>>
>>>
>>>
>>>
>>>
>>> On Apr 29, 2008, at 6:38 PM, Mon P Wang wrote:
>>>
>>>> Currently, llvm support add, swap, and the compare and swap.  I
>>>> want to add support for atomic support for sub, and, or, xor, min
>>>> and max (the latter two in both signed and unsigned version) in X86
>>>> line.  The intrinsic names are of the form
>>>> __sync_fetch_and_[opname] like other sync intrinsics.  If you have
>>>> any issues or concerns, please let me know.
>>>>
>>>> <atomic_op.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