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

[Roland Westrelin]

Hi John,

For 1. et 2. below:

> 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?

You're commenting this code change:

2866   if ((kind == xchg || kind == xadd) && type == T_LONG) {
2867     push_pair(load_store);
2868   } else {
2869     push(load_store);
2870   }

right?

Then for 2.: when kind == cmpxchg, then the else part is executed so load_store is pushed as a result.

and for 1.: when type is T_LONG for xchg or xadd, a pair is pushed but for cmpxchg a single result is pushed. So push_node wouldn't work, right?

> 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.

Ok.

> 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.)

Ok.

Roland.