Page MenuHomePhabricator

IR support for "cmpxchg weak"
ClosedPublic

Authored by t.p.northover on Jun 13 2014, 6:09 AM.

Details

Reviewers
chandlerc
Summary

Hi all,

Given the size & scope of this change, it's definitely worth getting the details checked over as much as possible. The patch here is substantially the same as the one in the RFC thread, but tidied up and with a bit more testing.

This patch is minimal, in the sense that it exposes some interfaces to make "cmpxchg weak" usable, but doesn't actually do anything with them. It comes with a similarly minimal Clang patch that just makes it correctly use the strong cmpxchg again.

I'm expecting 3 following patches, though they'll all be fairly trivial and obvious once this is in place:
+ Use the "weak" in AtomicExpandLoadLinked. This gives us actual benefit from the construct.
+ Use the i1 in X86ISelLowering.cpp. This lets us skip some redundant comparisons.
+ Make clang emit "cmpxchg weak" when appropriate. It's almost all wired up already; just a few extra lines and a bunch of tests.

OK to commit?

Tim.

Diff Detail

Event Timeline

t.p.northover retitled this revision from to IR support for "cmpxchg weak".
t.p.northover updated this object.
t.p.northover edited the test plan for this revision. (Show Details)
t.p.northover added a subscriber: Unknown Object (MLST).
chandlerc accepted this revision.Jun 13 2014, 6:39 AM
chandlerc added a reviewer: chandlerc.
chandlerc added a subscriber: chandlerc.

Lots of little nits, nothing substantial. LGTM, happy for you to commit in the granularity you want with as many of the fixes as seem good, none are essential or really necessary prior to it going in...

docs/Atomics.rst
432–433

This is an unrelated cleanup, you could submit separately if you want.... but maybe not worth splitting patches out.

docs/LangRef.rst
5086–5089

This one too.

5107–5110

The "Otherwise" here is hard for me to read because of the preceding clause. Maybe just re-state the inverse predicate?

5122

I really wonder why there were {i32}s throughout this.... Those could also go in a separate cleanup patch. At this point, that might be worthwhile, I see numerous other fixes of {i32} below for atomicrmw.

lib/AsmParser/LLParser.cpp
4259

'weak'?

4270

oh my, we already had the keyword from linkage...

should we update any of the organization of these in the LLLexer / LLToken code to make it less confusing that this keyword is used both as an instruction flag and a linkage spec keyword? anything else work this way already?

lib/Bitcode/Reader/BitcodeReader.cpp
2958

matching {}s on both sides of the else?

lib/CodeGen/AtomicExpandLoadLinkedPass.cpp
325–326

Range based loop?

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
142

Weird spacing here, just put break on its own line?

lib/Transforms/Instrumentation/ThreadSanitizer.cpp
535–539

Might want to talk with TSan folks about changing the interface in similar ways to remove the icmp....

test/CodeGen/PowerPC/Atomics-32.ll
532–533

your clever naming here entertains me. =]

This revision is now accepted and ready to land.Jun 13 2014, 6:39 AM
  • Original Message -----

From: "Tim Northover" <t.p.northover@gmail.com>
To: "t p northover" <t.p.northover@gmail.com>
Cc: llvm-commits@cs.uiuc.edu
Sent: Friday, June 13, 2014 8:09:33 AM
Subject: [PATCH] IR support for "cmpxchg weak"

Hi all,

Given the size & scope of this change, it's definitely worth getting
the details checked over as much as possible. The patch here is
substantially the same as the one in the RFC thread, but tidied up
and with a bit more testing.

This patch is minimal, in the sense that it exposes some interfaces
to make "cmpxchg weak" usable, but doesn't actually do anything with
them. It comes with a similarly minimal Clang patch that just makes
it correctly use the strong cmpxchg again.

This somewhat concerns me... does this make 'weak' the new default? If that is not backward compatible then maybe we should flip the default?

-Hal

I'm expecting 3 following patches, though they'll all be fairly
trivial and obvious once this is in place:
+ Use the "weak" in AtomicExpandLoadLinked. This gives us actual
benefit from the construct.
+ Use the i1 in X86ISelLowering.cpp. This lets us skip some redundant
comparisons.
+ Make clang emit "cmpxchg weak" when appropriate. It's almost all
wired up already; just a few extra lines and a bunch of tests.

OK to commit?

Tim.

http://reviews.llvm.org/D4134

Files:

docs/Atomics.rst
docs/LangRef.rst
include/llvm/CodeGen/ISDOpcodes.h
include/llvm/CodeGen/SelectionDAG.h
include/llvm/CodeGen/SelectionDAGNodes.h
include/llvm/IR/Instructions.h
lib/AsmParser/LLParser.cpp
lib/Bitcode/Reader/BitcodeReader.cpp
lib/Bitcode/Writer/BitcodeWriter.cpp
lib/CodeGen/AtomicExpandLoadLinkedPass.cpp
lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
lib/CodeGen/SelectionDAG/LegalizeTypes.h
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
lib/CodeGen/TargetLoweringBase.cpp
lib/IR/AsmWriter.cpp
lib/IR/Instruction.cpp
lib/IR/Instructions.cpp
lib/Target/CppBackend/CPPBackend.cpp
lib/Target/X86/X86ISelLowering.cpp
lib/Transforms/IPO/MergeFunctions.cpp
lib/Transforms/Instrumentation/ThreadSanitizer.cpp
lib/Transforms/Scalar/LowerAtomic.cpp
test/Assembler/atomic.ll
test/Bitcode/atomic.ll
test/Bitcode/memInstructions.3.2.ll
test/Bitcode/weak-cmpxchg-upgrade.ll
test/Bitcode/weak-cmpxchg-upgrade.ll.bc
test/CodeGen/AArch64/arm64-atomic-128.ll
test/CodeGen/AArch64/arm64-atomic.ll
test/CodeGen/AArch64/atomic-ops.ll
test/CodeGen/AArch64/cmpxchg-idioms.ll
test/CodeGen/ARM/atomic-64bit.ll
test/CodeGen/ARM/atomic-cmp.ll
test/CodeGen/ARM/atomic-op.ll
test/CodeGen/ARM/atomic-ops-v8.ll
test/CodeGen/ARM/cmpxchg-idioms.ll
test/CodeGen/CPP/atomic.ll
test/CodeGen/Mips/atomic.ll
test/CodeGen/Mips/atomicops.ll
test/CodeGen/PowerPC/Atomics-32.ll
test/CodeGen/PowerPC/atomic-1.ll
test/CodeGen/PowerPC/atomic-2.ll
test/CodeGen/R600/atomic_cmp_swap_local.ll
test/CodeGen/SPARC/atomics.ll
test/CodeGen/SystemZ/cmpxchg-01.ll
test/CodeGen/SystemZ/cmpxchg-02.ll
test/CodeGen/SystemZ/cmpxchg-03.ll
test/CodeGen/SystemZ/cmpxchg-04.ll
test/CodeGen/X86/2010-10-08-cmpxchg8b.ll
test/CodeGen/X86/Atomics-64.ll
test/CodeGen/X86/atomic_op.ll
test/CodeGen/X86/coalescer-remat.ll
test/Instrumentation/MemorySanitizer/atomics.ll
test/Transforms/AtomicExpandLoadLinked/ARM/atomic-expansion-v7.ll
test/Transforms/AtomicExpandLoadLinked/ARM/atomic-expansion-v8.ll
test/Transforms/LowerAtomic/atomic-swap.ll

llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

t.p.northover closed this revision.Jun 26 2014, 6:32 AM
jfb added a subscriber: jfb.Oct 21 2014, 9:37 PM

This seems to have caused a bug with flag clobber.