RFR (M): 7023898: Intrinsify AtomicLongFieldUpdater.getAndIncrement()
john.r.rose at oracle.com
Mon May 21 09:29:20 PDT 2012
On May 15, 2012, at 9:10 AM, Roland Westrelin wrote:
The C2 part looks good. I have a few small suggestions.
1. Replace push(cas) by push_node(type, load_store).
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 load_store_kind.
4. Naming: Consider using a more type-like name for load_store_kind, such as LoadStoreKind. Compare ReexecuteState and NodeType enums. Also, the enum names themselves could use a prefix or camel-case, e.g., LS_xadd or _xadd or XAdd. Since the names are private to library_call.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.)
Overall, it is a nicely rounded extension, with good reuse of previously existing code. Thanks!
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-compiler-dev