RFR (M): 8184334: Generalizing Atomic with templates

Fri Jul 28 16:01:10 UTC 2017

Hi Andrew,

I am glad we are converging. It seems we agree that we should go with 
the current new frontend that in a safe way connects existing users of 
the Atomic frontend to the existing backends of Atomic we have today 
(that uses truncated types), while sanity checking that new uses make 
sense and are safe. At the same time, it allows for the mechanism to be 
specialized with all type information available on a per platform basis 
in future backends as separate new RFEs where applicable, without 
requiring any changes to the shared frontend, by declaring a 
PlatformAtomic in an atomic platform header that arbitrarily overrides 
any Atomic operation completely with all type information preserved. I 
do hope this will make everyone happy involved.

Here is a refined full webrev with that concept (polished) from yesterday:
http://cr.openjdk.java.net/~eosterlund/8184334/webrev.03/

Incremental webrev from the initial webrev I sent out:
http://cr.openjdk.java.net/~eosterlund/8184334/webrev.01_03/

I hope you like this. I think you already indicated that you did. Please 
be aware also that I am going on vacation now and will be back in 3 
weeks time.

Thanks,
/Erik

On 2017-07-28 16:15, Andrew Haley wrote:
> Hi,
>
> On 27/07/17 18:27, Erik Osterlund wrote:
>> Hi Andrew,
>>
>>> On 27 Jul 2017, at 17:35, Andrew Haley <aph at redhat.com> wrote:
>>>
>>> Hi,
>>>
>>>> On 27/07/17 15:54, Erik Österlund wrote:
>>> Well, I think that was you, not me: it's simpler for everyone if we
>>> insist that all of the arguments to these functions are the same type.
>>> Some of the callers will have to change, sure.
>> I did indeed introduce the ability to match any types, but with
>> applied sanity checks protecting users from being able to compile if
>> incompatible types are passed in.
>>
>> I did try earlier on to see what would happen if we forced the types
>> to be identical as you propose. That turned out to be too strict. It
>> made it impossible to pass in perfectly fine literals like NULL or 0
>> where it should be fine (among other things). This would force too
>> many call sites to have to be changed to explicitly cast passed in
>> values to types that they could perfectly safely convert to
>> implicitly. Therefore I decided against it.
> OK.  It looked to me like some much-needed discipline for HotSpot
> programmers, but I guess that's a matter of taste.  :-)
>
>>>> Similarly, I believe similar problems may apply when different
>>>> integer types of with different sizes are passed in that would not
>>>> be implicitly convertible, but sure are explicitly convertible (with
>>>> possibly unintentional truncation). You mentioned the case of
>>>> extending a 32 bit integer to a 64 bit integer, which indeed is
>>>> fine. Unfortunately, the reverse is not fine yet possible in this
>>>> API. For example, if the destination is of type volatile jint* and
>>>> the passed in expected value is a jlong, then that is clearly
>>>> wrong. Yet the API will accept these types and explicitly convert
>>>> the jlong to jint, truncating it in the process without
>>>> warning. This is the kind of thing that the casting mechanism me and
>>>> Kim worked on prevents you from doing accidentally.
>>> OK, I see.  But it's a heck of a lot of code just to do that.
>> Perhaps. But IntegerTypes is a general reusable tool that might find
>> further use later on.
> Hmm.  This too is perhaps a matter of taste: I'm a great believer in
> not speculatively coding.  But that's a discussion for another day.
>
>>> Instead, we could simply insist that the types must be the same, and
>>> fix the callers.  This makes everything clearer, including the point
>>> where the Atomic API is called.  Mismatched types at this level are
>>> either unintentional oversights or plain bugs.  I have made this
>>> change, and it took a few minutes to fix all eight of the callers
>>> in HotSpot which have mismatched types.
>> I think I already covered why this was not done. Also, that is also
>> a "heck of a lot of code" invested in being able to reduce the
>> anount of code in Atomic. I think you will come to the same
>> conclusion once you apply that pattern to all atomic operations as
>> well as OrderAccess (coming up after we are done here as it builds
>> on Atomic).
> I believe not.
>
>>>> How would you feel about a mechanism where we allow each platform to
>>>> specialize on the top level with all type information available, but
>>>> by default send everything in to the Atomic implementation I have
>>>> proposed that canonicalizes and sanity checks all values. Then we
>>>> have solved a bunch of existing problems, not introduced any new
>>>> problems, yet are future proof for allowing arbitrary specialization
>>>> on each platform to allow further improving the solution on
>>>> platforms that have cooperative compilers that allow such
>>>> improvements, on a platform by platform basis.
>>>>
>>>> What do you think?
>>> It's a better solution, and it is much more well suited to the future,
>>> when we can move over to compiler intrinsics.  That's really
>>> important, IMO: it gives us an escape hatch from the past.
>> Okay. I am currently on my way home, but here is an untested prototype:
>>
>> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.02/
> Alright, let's see where this takes us.  It's be good to see if we can
> get the desirable properties of only allowing safe type conversions
> but still passing all of the types down to the native layer where
> possible.
>
>>> However, this would certainly fail to compile if anyone passed a
>>> double.  To handle floating-point operands I would do one of two
>>> things:
>>>
>>> Create explicit specializations for doubles and floats: these do
>>> bitwise copies into integer values when calling the assembly-language
>>> sinippets, or
>>>
>>> Make all of the snippets copy their arguments bitwise into integer
>>> values and back.  I believe that the best-supported way of doing that
>>> is the "union trick", which is widely supported by C++ compilers, even
>>> when using strict aliasing.
>> That makes sense. Unfortunately though, that would still violate
>> section 3.10 and remain undefined behaviour.
> It certainly wouldn't with GCC, because GCC defines that as an
> extension, and it is thus implementation-defined.  I suspect that is
> true of other compilers used to build HotSpot.  I don't much care
> about theoretical niceties, but I do care about incorrect code.  The
> union trick is so widely used that its UB may not matter, but I don't
> have much experience of proprietary C++ compilers so I might be wrong
> about that.
>
> But never mind, we're using -fno-strict-aliasing anyway.
>
>> Hope we are converging!
> I think so.
>