PATCH: using mixed types in MIN2/MAX2 functions

Dan Horák dan at danny.cz
Thu Jun 19 13:06:54 UTC 2014 

On Thu, 19 Jun 2014 14:55:52 +0200
Bengt Rutisson <bengt.rutisson at oracle.com> wrote:

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

if I remember correctly from my previous experience, these 2 definition
conflict with the existing
inline uint MAX2(uint a, uint b)
on platforms where size_t == uint. But I might be wrong, the easiest we
can do, is to try add them. Or we could add the new definitions only
for s390 with #if defined(__s390__) && ! defined(__s390x__). Or maybe
there is another way to add them only when size_t != uint. A C++ expert
is required :-)


		Dan

> 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