PATCH: using mixed types in MIN2/MAX2 functions

Bengt Rutisson bengt.rutisson at oracle.com
Thu Jun 19 12:55:52 UTC 2014 

Jesper,

Thanks for uploading the webrev on the openjdk server.

Dan,

Can you explain more in detail why it is not possible to specialize the 
MIN2 and MAX2 functions? You are probably correct, because when I read 
the comment in the code it says:


// It is necessary to use templates here. Having normal overloaded
// functions does not work because it is necessary to provide both 32-
// and 64-bit overloaded functions, which does not work, and having
// explicitly-typed versions of these routines (i.e., MAX2I, MAX2L)
// will be even more error-prone than macros.
template<class T> inline T MAX2(T a, T b)           { return (a > b) ? a 
: b; }


This kind of says what you also said. That it is not possible, but it 
does not really explain why.

Can you explain why we can have definitions like:

inline uint MAX2(uint a, size_t b)
inline uint MAX2(size_t a, uint b)

to complement the present template version?

Thanks,
Bengt

On 6/18/14 2:47 PM, Jesper Wilhelmsson wrote:
> Hi Dan,
>
> I have uploaded your webrev here:
>
> http://cr.openjdk.java.net/~jwilhelm/8046938/webrev/
>
> The change looks good to me.
> /Jesper
>
>
> Dan Horák skrev 18/6/14 08:55:
>> On Mon, 16 Jun 2014 19:01:20 +0200
>> Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>
>>> Hi Dan,
>>>
>>> I created a bug four this issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8046938
>>> /Jesper
>>
>> updated webrev is now at http://fedora.danny.cz/openjdk/size_t-2/webrev/
>> (and http://fedora.danny.cz/openjdk/size_t-2/webrev.zip)
>>
>> changes:
>> - refreshed for current hs-gc repository
>> - dropped unnecessary typecast in get_hum_bytes() (concurrentMark.cpp)
>> - typecasts in FLAG_SET_ERGO() changed to uintx from size_t
>>     (arguments.cpp)
>>
>>
>>     With regards
>>
>>         Dan
>>
>>> Dan Horák skrev 16/6/14 18:29:
>>>> On Mon, 16 Jun 2014 17:26:24 +0200
>>>> Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>>>
>>>>> Hi Dan,
>>>>>
>>>>> Since you got a comment from Bengt maybe you should fix that issue
>>>>> before I upload the new webrev.
>>>>>
>>>>> I looked through the webrev and I have one additional comment. I
>>>>> would prefer if the changes in arguments.cpp was the other way,
>>>>> casting stuff to uintx instead of size_t, since we use the result
>>>>> of MAX and MIN to set flags of type uintx.
>>>>
>>>> I wasn't sure what would be better approach here
>>>>
>>>>> When I applied the webrev a change in
>>>>> src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp got in the
>>>>> way of the patch. The patch still applies but with an offset, so
>>>>> patch complains. If you build a new webrev with the changes above,
>>>>> also pull down the latest hs-gc to get the g1CollectedHeap.cpp
>>>>> change.
>>>>
>>>> thanks for the comments, I'll prepare new webrev hopefully tomorrow
>>>>
>>>>
>>>>         Dan
>>>>
>>>>> Thanks!
>>>>> /Jesper
>>>>>
>>>>>
>>>>> Dan Horák skrev 16/6/14 16:21:
>>>>>> On Mon, 16 Jun 2014 16:19:17 +0200
>>>>>> Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>>>>>
>>>>>>> Dan,
>>>>>>>
>>>>>>> I can host the webrev. Send me the zipped webrev archive and I'll
>>>>>>> upload it to cr.openjdk.
>>>>>>
>>>>>> Jesper, thanks a lot, the whole archive is at
>>>>>> http://fedora.danny.cz/openjdk/size_t-1/webrev.zip
>>>>>>
>>>>>>
>>>>>>         Dan
>>>>>>
>>>>>>> /Jesper
>>>>>>>
>>>>>>> David Holmes skrev 16/6/14 13:15:
>>>>>>>> On 16/06/2014 5:17 PM, Dan Horák wrote:
>>>>>>>>> On Mon, 16 Jun 2014 16:31:29 +1000
>>>>>>>>> David Holmes <david.holmes at oracle.com> wrote:
>>>>>>>>>
>>>>>>>>>> On 13/06/2014 10:55 PM, Dan Horák wrote:
>>>>>>>>>>> On Fri, 13 Jun 2014 14:10:53 +0200
>>>>>>>>>>> Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
>>>>>>>>>>>> Then a formality comment. I think you have to post the
>>>>>>>>>>>> webrev on cr.openjdk.java.net to get the legal stuff
>>>>>>>>>>>> correct.
>>>>>>>>>>>
>>>>>>>>>>> hm, I've used this procedure (my webrev, then someone
>>>>>>>>>>> privileged takes it and pushes it) few times in the past and
>>>>>>>>>>> it worked.
>>>>>>>>>>
>>>>>>>>>> We've been advised that all contributions have to come in via
>>>>>>>>>> OpenJDK infrastructure - either a webrev on
>>>>>>>>>> cr.openjdk.java.net or as attachments to mailing list mails.
>>>>>>>>>
>>>>>>>>> I understand, but AFAIK cr.openjdk.java.net is accessible only
>>>>>>>>> to people with push access, which I'm not.
>>>>>>>>
>>>>>>>> No you only need Author status, not Committer.
>>>>>>>>
>>>>>>>>> So the question is will the list
>>>>>>>>> accept 3MB attachment? It is the size of the zipped webrev. Or
>>>>>>>>> will a plain text patch suffice?
>>>>>>>>
>>>>>>>> For large webrevs you may be able to find someone to host it for
>>>>>>>> you on cr.openjdk. I don't know what limit exists on the mailing
>>>>>>>> list but I'd hate to see very large patches submitted that way.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> David
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>            Dan
>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>> -----
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>            Dan
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Bengt
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2014-06-13 09:55, Dan Horák wrote:
>>>>>>>>>>>>> On Thu, 12 Jun 2014 21:49:40 +0200
>>>>>>>>>>>>> Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Which repository is the patch built on? Since it is mostly
>>>>>>>>>>>>>> GC code it would be good if it goes in through jdk9/hs-gc.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The changes looks good in the webrev, but the patch did
>>>>>>>>>>>>>> not apply to a recent jdk9/hs-gc. Especially the collector
>>>>>>>>>>>>>> policy code has changed a lot recently.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I would prefer to look at the changes applied in my
>>>>>>>>>>>>>> workspace before approving it, so if you could update to a
>>>>>>>>>>>>>> recent hs-gc it would be great.
>>>>>>>>>>>>> I have refreshed the patch for the hs-gc repository, please
>>>>>>>>>>>>> see http://fedora.danny.cz/openjdk/size_t-1/webrev/
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>        Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>>            Dan
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>> /Jesper
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Vladimir Kozlov skrev 12/6/14 20:57:
>>>>>>>>>>>>>>> Thank you, Dan
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I think casting is acceptable solution. Someone in GC
>>>>>>>>>>>>>>> group should review and sponsor this since you touched
>>>>>>>>>>>>>>> GC code mostly.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>> Vladimir
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 6/12/14 2:05 AM, Dan Horák wrote:
>>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> this the last and largest part of my changes required on
>>>>>>>>>>>>>>>> s390 (32-bit) to build OpenJDK out of the box. The
>>>>>>>>>>>>>>>> MIN2/MAX2 functions are implemented using templates and
>>>>>>>>>>>>>>>> require both arguments to be of the same type. This is a
>>>>>>>>>>>>>>>> problem when one parameter is of size_t type and the
>>>>>>>>>>>>>>>> second of uintx type and the platform has size_t defined
>>>>>>>>>>>>>>>> as eg. unsigned long as on s390 (32-bit).
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> As a solution [1] I chose to typecast the smaller type
>>>>>>>>>>>>>>>> (uintx) to size_t which is of same or larger size.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> [1] http://fedora.danny.cz/openjdk/size_t/webrev/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Note: I'm Red Hat employee (dhorak at redhat.com), so I
>>>>>>>>>>>>>>>> should be covered by Red Hat's CLA for my contributions.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>           With regards
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>               Dan
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>

More information about the hotspot-dev mailing list