RFR (M): 7023898: Intrinsify AtomicLongFieldUpdater.getAndIncrement() (original) (raw)

Vladimir Kozlov vladimir.kozlov at oracle.com
Mon Jun 25 12:08:51 PDT 2012

Even so John said you can use LS I would suggest to use full name. Why not use nodes names? For a example: GetAndAdd, GetAndSet, CompareAndSwap. It is easier to understand the meaning.

The rest is good.

Thanks, Vladimir

Roland Westrelin wrote:

Here is a new webrev: http://cr.openjdk.java.net/~roland/7023898/webrev.01/

that takes these suggestions into account:

The C2 part looks good. I have a few small suggestions.

1. Replace push(cas) by pushnode(type, loadstore). 2. I don't understand why the (kind = cmpxchg) case doesn't push a result. Does't the original push(cas) do that? 3. In the if/else chain testing 'kind', change the bare 'else' to 'else if (kind ≡ cmpxchg)' and follow up with 'else ShouldNotReachHere()'. At least put an "assert(kind ≡ cmpxchg)". This will ease maintenance, in case somebody adds a new case to loadstorekind. 4. Naming: Consider using a more type-like name for loadstorekind, such as LoadStoreKind. Compare ReexecuteState and NodeType enums. Also, the enum names themselves could use a prefix or camel-case, e.g., LSxadd or xadd or XAdd. Since the names are private to librarycall.cpp, it doesn't matter as much as in other places, but it still improves readability of code that contains the names. (Reason: an all-lower-case simple name like xadd appears at first glance to be a local variable. Manifest constants should look different.) Roland.

More information about the hotspot-compiler-dev mailing list