This is an archive of the discontinued LLVM Phabricator instance.

[IR] Extend cmpxchg to allow pointer type operands
ClosedPublic

Authored by reames on Feb 18 2016, 1:48 PM.

Details

Summary

Today, we do not allow cmpxchg operations with pointer arguments. We require the frontend to insert ptrtoint casts and do the cmpxchg in integers. While correct, this is problematic from a couple of perspectives:

  1. It makes the IR harder to analyse (for instance, it make capture tracking overly conservative)
  2. It pushes work onto the frontend authors for no real gain

This patch implements the simplest form of IR support. As we did with floating point loads and stores, we teach AtomicExpand to convert back to the old representation. This prevents us needing to change all backends in a single lock step change. Over time, we can migrate each backend to natively selecting the pointer type. In the meantime, we get the advantages of a cleaner IR representation without waiting for the backend changes.

Diff Detail

Event Timeline

reames updated this revision to Diff 48400.Feb 18 2016, 1:48 PM
reames retitled this revision from to [IR] Extend cmpxchg to allow pointer type operands.
reames updated this object.
reames added reviewers: jfb, hfinkel.
reames added a subscriber: llvm-commits.
jfb added a subscriber: theraven.Feb 18 2016, 1:55 PM
jfb added inline comments.Feb 18 2016, 2:08 PM
lib/AsmParser/LLParser.cpp
5912

I think this is too permissive, store does:

if (!Val->getType()->isFirstClassType())
  return Error(Loc, "store operand must be a first class value");

The change is also allowing pointer types. Should this be a separate change? At the minimum it has to be tested.

lib/CodeGen/AtomicExpandPass.cpp
173

FP can fall into the same helper? Though the helper will need to support FP as well.

467

"equivalent"

test/Transforms/AtomicExpand/X86/expand-atomic-non-integer.ll
124

Add a test with volatile and addrspace, make sure they're kept.

reames added inline comments.Feb 18 2016, 2:26 PM
lib/AsmParser/LLParser.cpp
5912

I was a bit confused why this code was here at all. Shouldn't this just be caught by the Verifier? It seems to have the checks duplicated twice.

lib/CodeGen/AtomicExpandPass.cpp
173

That's the intent. I just wanted to do that as a separate change.

test/Transforms/AtomicExpand/X86/expand-atomic-non-integer.ll
124

will do

jfb added inline comments.Feb 18 2016, 2:46 PM
lib/AsmParser/LLParser.cpp
5912

I'm not sure which checks are expected to go in which part of LLVM :-)

The verifier seems like more of a sanity check for internals (an assert) whereas this part seems to be the syntax check on .ll files themselves (equivalent to the errors clang gives)?

It's redundant, but there seems to be a purpose to the redundancy.

lib/CodeGen/AtomicExpandPass.cpp
173

Ah OK, and it'll fail in the verifier so it's fine. Maybe just assert early here? It'll fail miserably in convertCmpXchgToIntegerType otherwise.

reames updated this revision to Diff 48413.Feb 18 2016, 3:26 PM

Address JF's review comments.

jfb accepted this revision.Feb 18 2016, 3:36 PM
jfb edited edge metadata.

lgtm

This revision is now accepted and ready to land.Feb 18 2016, 3:36 PM
This revision was automatically updated to reflect the committed changes.
theraven added inline comments.Feb 19 2016, 4:16 AM
llvm/trunk/docs/LangRef.rst
7091 ↗(On Diff #48420)

Is there a reason to disallow floating point types? C11 expects compare and exchange to work on floating point types, and requiring casts to integer types doesn't play particularly nicely with TBAA.

Given the recent mailing list discussion, is this part of the work to allow the front end to provide any type here and lower to a library call closer to the back end?

llvm/trunk/lib/CodeGen/AtomicExpandPass.cpp
179 ↗(On Diff #48420)

This TODO is quite important for us. We're currently unable to use or pointer LL/SC instructions from C because our target does not have a valid integer type as big as our pointer type.