[PATCH] Use the load-acquire/store-release instructions optimally in AArch32

Tim Northover t.p.northover at gmail.com
Wed Sep 11 03:45:36 PDT 2013


Hi Artyom,

Very nice ideas, the code manages to do more and look neater at the
end of it, I think. Thanks for looking at this.

Some comments:

 /// AtomicSDNode - A SDNode reprenting atomic operations.
 ///
 class AtomicSDNode : public MemSDNode {
-  SDUse Ops[4];
+  SDUse Ops[6];

Together with how allocation works, I think this means that every
SDNode LLVM allocates will be substantially larger after this change.
Is that justified?

Looking at SelectionDAGNodes.h, there's some facility for allocating
the operands on the heap ("new SDUse" in one of the SDNode
constructors), and given the rarity of atomic nodes did you try using
that?

+    DAG.getAtomic(Node->getOpcode(), dl, MVT::i64, Tys, Ops.data(), Ops.size(),
+                  cast<MemSDNode>(Node)->getMemOperand(), AN->getOrdering(),
+                  AN->getSynchScope());

This is very appealing indeed. It's extending the semantics of a
generic node though. In principle DAGCombiner could be expecting a
single legal type and trying to optimise (it's not at the moment).

Personally, I think the neatness gain makes it worthwhile, but
together with the above, I think we should try to get a real
SelectionDAG expert to weigh in on this patch. I've added Owen, who
seems to have been lumbered with it in CODE_OWNERS.txt.

+  let mayLoad = 1, mayStore = 1 in {

Why are you only marking the 64-bit operations with mayLoad and mayStore?

+++ b/test/CodeGen/ARM/atomic-ops.ll

Might be an idea to put v8 in the filename so people know what's being tested.

Cheers.

Tim.



More information about the llvm-commits mailing list