[llvm-commits] atomic operator patch

[Evan Cheng]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: op.atomic-add
Type: application/octet-stream
Size: 5745 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080430/7c2b95ed/attachment.obj>
-------------- next part --------------



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