Page MenuHomePhabricator

Add two helper functions: isAtLeastAcquire, isAtLeastRelease
ClosedPublic

Authored by morisset on Aug 11 2014, 10:36 AM.

Details

Reviewers
reames
jfb
Summary

These methods are available to loads/stores respectively, and will be used
in a later separate patch.
This is patch 2/3 of the work started in D4797, but should be completely independent.

Diff Detail

Event Timeline

morisset updated this revision to Diff 12349.Aug 11 2014, 10:36 AM
morisset retitled this revision from to Add two helper functions: isAtLeastAcquire, isAtLeastRelease.
morisset updated this object.
morisset edited the test plan for this revision. (Show Details)
morisset added reviewers: jfb, reames.
morisset added a subscriber: Unknown Object (MLST).
jfb edited edge metadata.Aug 11 2014, 10:47 AM

Shouldn't both also accept acq_rel?

If they were for generic instructions, yes (but generic instructions don't have a MemoryOrdering).
Here they are for loads/stores, which cannot have the acq_rel memory ordering (as said in a comment a few lines before each of my changes).
If this patch is later extended to RMW instructions, then yes acq_rel will have to be taken into account.

jfb added a comment.Aug 11 2014, 10:57 AM
In D4844#5, @morisset wrote:

If they were for generic instructions, yes (but generic instructions don't have a MemoryOrdering).
Here they are for loads/stores, which cannot have the acq_rel memory ordering (as said in a comment a few lines before each of my changes).
If this patch is later extended to RMW instructions, then yes acq_rel will have to be taken into account.

Oh right. Could you add a short comment in each function body?

morisset updated this revision to Diff 12354.Aug 11 2014, 11:03 AM
morisset edited edge metadata.

Add a comment to the body of the two functions, based on the comment of jfb

jfb accepted this revision.Aug 11 2014, 11:05 AM
jfb edited edge metadata.

This lgtm, I'll leave it open for a while for others to comment, then I'll commit.

This revision is now accepted and ready to land.Aug 11 2014, 11:05 AM
reames edited edge metadata.Aug 12 2014, 12:15 PM

Generally, we prefer that functions like this are used in the codebase. It's acceptable to have closely following patches, but I see a couple places you could use these today.

for example: Arch64ISelLowering.cpp could make use of isAcquireOrdering.

As a high level structure, I'd suggest making these predicates on orderings, not predicates on instructions. This would remove a lot of the complexity around which instructions legally have which orders. This also seems to be how existing code is structured.

LGTM with inline comments addressed. The structural comments above is optional.

include/llvm/IR/Instructions.h
255

Please add a doxygen comment describing what this does. I might also pick a different name. Most readers won't know that acquire refers to memory ordering. Possible: "isAcquireOrdered", "isOrderedAcquireOrStronger", etc..

256

I would prefer you simply checked for AcquireRelease. While it might be dead code for a Load, it decrease the odds of a bug if this code is copied elsewhere. Alternatively, turn the comment into an assert that the ordering is not AcquireRelease.

375

Same comments as above.

morisset updated this revision to Diff 12424.Aug 12 2014, 5:05 PM
morisset edited edge metadata.

I moved the two helpers from being methods of LoadInst/StoreInst to being
free-standing functions operating on AtomicOrdering, on the suggestion of Philip Reames,
it is indeed much clearer.
Also added a doxygen comment (although it does not tell much more than the code..)
I did not change the name of the function, I hope it is more self-explanatory now that
they have an AtomicOrdering in their signature.
Using them elsewhere in the codebase will come in another patch if this one lands.

With minor nitpicks, LGTM.

include/llvm/IR/Instructions.h
53

You should drop the anon namespace here.

57

Stylistic:
"( A ||

B)"

Neither is "better", but this is the norm in LLVM code. Please follow it.

morisset updated this revision to Diff 12465.Aug 13 2014, 1:10 PM

Stylistic fixes to isAtLeastAcquire/isAtLeastRelease

morisset closed this revision.Aug 15 2014, 3:35 PM

commited in r215779.