RFR (M): 7023898: Intrinsify AtomicLongFieldUpdater.getAndIncrement()
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 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.)
>
> Roland.
More information about the hotspot-compiler-dev
mailing list