[llvm-commits] atomic operator patch
Dan Gohman
gohman at apple.com
Wed Apr 30 08:31:31 PDT 2008
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
>
More information about the llvm-commits
mailing list