[llvm-commits] atomic operator patch

[Dan Gohman]

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
>