Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-18 Thread Volker Simonis 
For the top-level change I always get a strange error when importing it:

patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 8062493_jvmci_top_0911.v1.patch

It is because of the strange path syntax of the last hunk in the patch file:

--- old/./modules.xml 2015-09-16 15:11:14.0 -0700
+++ new/./modules.xml 2015-09-16 15:11:14.0 -0700
@@ -254,6 +254,14 @@
   jdk.jfr
 
 
+  jdk.internal.jvmci.hotspot
+  jdk.jfr
+
+
+  jdk.internal.jvmci.hotspot.events
+  jdk.jfr
+
+
   sun.misc
   java.corba
   java.desktop


Notice the strange syntax "old/./modules.xml" and "new/./modules.xml".
If I remove the redundant './' from the path, everything works fine.
For some reason only the diffs for modules.xml has this strange path
syntax in the patch.

Regards,
Volker


On Thu, Sep 17, 2015 at 10:32 PM, Christian Thalinger
 wrote:
> Since there are no objections I’m going to push this…
>
> http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/6a6766a8cbbf
>
>> On Sep 16, 2015, at 3:05 PM, Christian Thalinger 
>>  wrote:
>>
>> I would like to add this change:
>>
>> diff -r 2134e08cc132 src/share/vm/utilities/vmError.cpp
>> --- a/src/share/vm/utilities/vmError.cpp  Wed Sep 16 14:28:33 2015 -1000
>> +++ b/src/share/vm/utilities/vmError.cpp  Wed Sep 16 15:04:02 2015 -1000
>> @@ -517,6 +517,10 @@ void VMError::report(outputStream* st) {
>>Abstract_VM_Version::vm_release(),
>>Abstract_VM_Version::vm_info_string(),
>>TieredCompilation ? ", tiered" : "",
>> +#if INCLUDE_JVMCI
>> +   EnableJVMCI ? ", jvmci" : "",
>> +   UseJVMCICompiler ? ", jvmci compiler" : "",
>> +#endif
>>UseCompressedOops ? ", compressed oops" : "",
>>gc_mode(),
>>Abstract_VM_Version::vm_platform_string()
>>
>> It would be useful to see in the crash logs if this experimental feature was 
>> turned on.
>>
>>> On Sep 16, 2015, at 12:43 PM, Vladimir Kozlov  
>>> wrote:
>>>
>>> I updated top and hotspot webrev with latest (build) changes.
>>>
>>> Vladimir
>>>
>>> On 9/16/15 2:28 PM, Christian Thalinger wrote:

> On Sep 16, 2015, at 6:52 AM, Christian Thalinger 
>  wrote:
>
>
>> On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie 
>>  wrote:
>>
>> On 2015-09-14 09:24, Christian Thalinger wrote:
>>> The JEP itself can be found here:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8062493 
>>> 
>>>
>>> Here are the webrevs:
>>>
>>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 
>>> 
>>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 
>>> 
>>>
>>> The change has already undergone a few iterations of internal review by 
>>> different people of different teams.  Most comments and suggestions 
>>> were handled accordingly.  Although there is one open item we agreed we 
>>> will address after the integration of JEP 243 and that work is captured 
>>> in RFE:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8134994 
>>> 
>>>
>>> SQE is still working on the tests and all test tasks can be seen as 
>>> sub-tasks of the JEP.
>>>
>>> The integration will happen under the bug number:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8136421 
>>> 
>>>
>> Hi Christian,
>>
>> (Adding build-dev since this review includes some major build changes.)
>>
>> The makefile changes looks good in general to me. I have not reviewed 
>> the source code changes.
>>
>> Some comments:
>>
>> * modules.xml:
>> Make sure you get sign-off from a Jigsaw developer for modifying this 
>> file.
>
> I’ve asked Alan to take a look.
>
>>
>> * hotspot/make/linux/makefiles/gcc.make:
>> Seems unfortunate to have to disable a new warning 
>> (undefined-bool-conversion) for newly incorporated code. Is it not 
>> possible to fix the code emitting this warning instead?
>
> We had this question before.  It’s not obvious because you can’t see it 
> in the regular diff views but this is under:
>
> ifeq ($(USE_CLANG), true)
>
>>
>> * make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
>> I see a coming merge conflict. In jdk9/dev, there is now a new function 
>> that performs the same function as CreatePath, but

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-18 Thread Christian Thalinger 

> On Sep 17, 2015, at 10:20 PM, Volker Simonis  wrote:
> 
> For the top-level change I always get a strange error when importing it:
> 
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working dir
> errors during apply, please fix and refresh 8062493_jvmci_top_0911.v1.patch
> 
> It is because of the strange path syntax of the last hunk in the patch file:
> 
> --- old/./modules.xml 2015-09-16 15:11:14.0 -0700
> +++ new/./modules.xml 2015-09-16 15:11:14.0 -0700
> @@ -254,6 +254,14 @@
>   jdk.jfr
> 
> 
> +  jdk.internal.jvmci.hotspot
> +  jdk.jfr
> +
> +
> +  jdk.internal.jvmci.hotspot.events
> +  jdk.jfr
> +
> +
>   sun.misc
>   java.corba
>   java.desktop
> 
> 
> Notice the strange syntax "old/./modules.xml" and "new/./modules.xml".
> If I remove the redundant './' from the path, everything works fine.
> For some reason only the diffs for modules.xml has this strange path
> syntax in the patch.

That’s odd.  Vladimir created the webrevs.  Maybe he did something different 
with the top-level one.

> 
> Regards,
> Volker
> 
> 
> On Thu, Sep 17, 2015 at 10:32 PM, Christian Thalinger
>  wrote:
>> Since there are no objections I’m going to push this…
>> 
>> http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/6a6766a8cbbf
>> 
>>> On Sep 16, 2015, at 3:05 PM, Christian Thalinger 
>>>  wrote:
>>> 
>>> I would like to add this change:
>>> 
>>> diff -r 2134e08cc132 src/share/vm/utilities/vmError.cpp
>>> --- a/src/share/vm/utilities/vmError.cpp  Wed Sep 16 14:28:33 2015 -1000
>>> +++ b/src/share/vm/utilities/vmError.cpp  Wed Sep 16 15:04:02 2015 -1000
>>> @@ -517,6 +517,10 @@ void VMError::report(outputStream* st) {
>>>   Abstract_VM_Version::vm_release(),
>>>   Abstract_VM_Version::vm_info_string(),
>>>   TieredCompilation ? ", tiered" : "",
>>> +#if INCLUDE_JVMCI
>>> +   EnableJVMCI ? ", jvmci" : "",
>>> +   UseJVMCICompiler ? ", jvmci compiler" : "",
>>> +#endif
>>>   UseCompressedOops ? ", compressed oops" : "",
>>>   gc_mode(),
>>>   Abstract_VM_Version::vm_platform_string()
>>> 
>>> It would be useful to see in the crash logs if this experimental feature 
>>> was turned on.
>>> 
 On Sep 16, 2015, at 12:43 PM, Vladimir Kozlov  
 wrote:
 
 I updated top and hotspot webrev with latest (build) changes.
 
 Vladimir
 
 On 9/16/15 2:28 PM, Christian Thalinger wrote:
> 
>> On Sep 16, 2015, at 6:52 AM, Christian Thalinger 
>>  wrote:
>> 
>> 
>>> On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie 
>>>  wrote:
>>> 
>>> On 2015-09-14 09:24, Christian Thalinger wrote:
 The JEP itself can be found here:
 
 https://bugs.openjdk.java.net/browse/JDK-8062493 
 
 
 Here are the webrevs:
 
 http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 
 
 http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 
 
 
 The change has already undergone a few iterations of internal review 
 by different people of different teams.  Most comments and suggestions 
 were handled accordingly.  Although there is one open item we agreed 
 we will address after the integration of JEP 243 and that work is 
 captured in RFE:
 
 https://bugs.openjdk.java.net/browse/JDK-8134994 
 
 
 SQE is still working on the tests and all test tasks can be seen as 
 sub-tasks of the JEP.
 
 The integration will happen under the bug number:
 
 https://bugs.openjdk.java.net/browse/JDK-8136421 
 
 
>>> Hi Christian,
>>> 
>>> (Adding build-dev since this review includes some major build changes.)
>>> 
>>> The makefile changes looks good in general to me. I have not reviewed 
>>> the source code changes.
>>> 
>>> Some comments:
>>> 
>>> * modules.xml:
>>> Make sure you get sign-off from a Jigsaw developer for modifying this 
>>> file.
>> 
>> I’ve asked Alan to take a look.
>> 
>>> 
>>> * hotspot/make/linux/makefiles/gcc.make:
>>> Seems unfortunate to have to disable a new warning 
>>> (undefined-bool-conversion) for newly incorporated code. Is it not 
>>> possible to fix the code emitting this warning instead?
>> 
>> We had this

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-17 Thread Magnus Ihse Bursie 

On 2015-09-16 22:25, Christian Thalinger wrote:


On Sep 16, 2015, at 10:11 AM, Magnus Ihse Bursie 
> wrote:


On 2015-09-16 18:52, Christian Thalinger wrote:
On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie 
> wrote:


On 2015-09-14 09:24, Christian Thalinger wrote:

The JEP itself can be found here:

https://bugs.openjdk.java.net/browse/JDK-8062493 



Here are the webrevs:

http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 
 
>
http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 
 
>


The change has already undergone a few iterations of internal 
review by different people of different teams.  Most comments and 
suggestions were handled accordingly. Although there is one open 
item we agreed we will address after the integration of JEP 243 
and that work is captured in RFE:


https://bugs.openjdk.java.net/browse/JDK-8134994 



SQE is still working on the tests and all test tasks can be seen 
as sub-tasks of the JEP.


The integration will happen under the bug number:

https://bugs.openjdk.java.net/browse/JDK-8136421 




Hi Christian,

(Adding build-dev since this review includes some major build changes.)

The makefile changes looks good in general to me. I have not 
reviewed the source code changes.


Some comments:

* modules.xml:
Make sure you get sign-off from a Jigsaw developer for modifying 
this file.

I’ve asked Alan to take a look.


* hotspot/make/linux/makefiles/gcc.make:
Seems unfortunate to have to disable a new warning 
(undefined-bool-conversion) for newly incorporated code. Is it not 
possible to fix the code emitting this warning instead?
We had this question before.  It’s not obvious because you can’t see 
it in the regular diff views but this is under:


ifeq ($(USE_CLANG), true)


I'm not sure I understand why that's relevant..? Isn't it just as 
important to try to submit warning-free code when compiling with 
clang as with any other compiler? Or is clang just being anal about 
the code in question?


I don’t have a Clang compiler at hand but Clang is anal about 
everything.  Do you want that suppression to be removed?


It's more a hotspot code quality issue, not a build system issue, so I 
won't insist. I just wanted to point out that this change will start 
hiding a new kind of warning for all files in hotspot. Unless there was 
a compelling reason, I would personally rather see an effort to fix the 
code in question. But if no-one from Hotspot agrees on this, I'll drop it.


/Magnus

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-17 Thread Christian Thalinger 
Since there are no objections I’m going to push this…

http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/6a6766a8cbbf

> On Sep 16, 2015, at 3:05 PM, Christian Thalinger 
>  wrote:
> 
> I would like to add this change:
> 
> diff -r 2134e08cc132 src/share/vm/utilities/vmError.cpp
> --- a/src/share/vm/utilities/vmError.cpp  Wed Sep 16 14:28:33 2015 -1000
> +++ b/src/share/vm/utilities/vmError.cpp  Wed Sep 16 15:04:02 2015 -1000
> @@ -517,6 +517,10 @@ void VMError::report(outputStream* st) {
>Abstract_VM_Version::vm_release(),
>Abstract_VM_Version::vm_info_string(),
>TieredCompilation ? ", tiered" : "",
> +#if INCLUDE_JVMCI
> +   EnableJVMCI ? ", jvmci" : "",
> +   UseJVMCICompiler ? ", jvmci compiler" : "",
> +#endif
>UseCompressedOops ? ", compressed oops" : "",
>gc_mode(),
>Abstract_VM_Version::vm_platform_string()
> 
> It would be useful to see in the crash logs if this experimental feature was 
> turned on.
> 
>> On Sep 16, 2015, at 12:43 PM, Vladimir Kozlov  
>> wrote:
>> 
>> I updated top and hotspot webrev with latest (build) changes.
>> 
>> Vladimir
>> 
>> On 9/16/15 2:28 PM, Christian Thalinger wrote:
>>> 
 On Sep 16, 2015, at 6:52 AM, Christian Thalinger 
  wrote:
 
 
> On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2015-09-14 09:24, Christian Thalinger wrote:
>> The JEP itself can be found here:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8062493 
>> 
>> 
>> Here are the webrevs:
>> 
>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 
>> 
>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 
>> 
>> 
>> The change has already undergone a few iterations of internal review by 
>> different people of different teams.  Most comments and suggestions were 
>> handled accordingly.  Although there is one open item we agreed we will 
>> address after the integration of JEP 243 and that work is captured in 
>> RFE:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8134994 
>> 
>> 
>> SQE is still working on the tests and all test tasks can be seen as 
>> sub-tasks of the JEP.
>> 
>> The integration will happen under the bug number:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8136421 
>> 
>> 
> Hi Christian,
> 
> (Adding build-dev since this review includes some major build changes.)
> 
> The makefile changes looks good in general to me. I have not reviewed the 
> source code changes.
> 
> Some comments:
> 
> * modules.xml:
> Make sure you get sign-off from a Jigsaw developer for modifying this 
> file.
 
 I’ve asked Alan to take a look.
 
> 
> * hotspot/make/linux/makefiles/gcc.make:
> Seems unfortunate to have to disable a new warning 
> (undefined-bool-conversion) for newly incorporated code. Is it not 
> possible to fix the code emitting this warning instead?
 
 We had this question before.  It’s not obvious because you can’t see it in 
 the regular diff views but this is under:
 
 ifeq ($(USE_CLANG), true)
 
> 
> * make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
> I see a coming merge conflict. In jdk9/dev, there is now a new function 
> that performs the same function as CreatePath, but it's named PathList. 
> (It's a bit unfortunate that this code snippet has bounced around as 
> patches without a definite name.) I assume you are going to push this 
> through a hotspot forest. If the PathList patch reaches the hotspot repo 
> before this, please remove the CreatePath from MakeBase, and change the 
> calls to CreatePath to PathList instead. (I could only find such calls in 
> hotspot/make/gensrc/Gensrc-java.base.gmk). If this patch goes in before 
> that, we'll need to give a heads-up to the integrator about this conflict.
 
 Thanks for the heads up.
>>> 
>>> Erik sent me a patch to avoid merge conflicts.  I’ve integrated two 
>>> changesets:
>>> 
>>> http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab 
>>> >>  >
>>> http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9 
>>>

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-16 Thread Magnus Ihse Bursie 

On 2015-09-14 09:24, Christian Thalinger wrote:

The JEP itself can be found here:

https://bugs.openjdk.java.net/browse/JDK-8062493 


Here are the webrevs:

http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 

http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 


The change has already undergone a few iterations of internal review by 
different people of different teams.  Most comments and suggestions were 
handled accordingly.  Although there is one open item we agreed we will address 
after the integration of JEP 243 and that work is captured in RFE:

https://bugs.openjdk.java.net/browse/JDK-8134994 


SQE is still working on the tests and all test tasks can be seen as sub-tasks 
of the JEP.

The integration will happen under the bug number:

https://bugs.openjdk.java.net/browse/JDK-8136421 



Hi Christian,

(Adding build-dev since this review includes some major build changes.)

The makefile changes looks good in general to me. I have not reviewed 
the source code changes.


Some comments:

* modules.xml:
Make sure you get sign-off from a Jigsaw developer for modifying this file.

* hotspot/make/linux/makefiles/gcc.make:
Seems unfortunate to have to disable a new warning 
(undefined-bool-conversion) for newly incorporated code. Is it not 
possible to fix the code emitting this warning instead?


* make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
I see a coming merge conflict. In jdk9/dev, there is now a new function 
that performs the same function as CreatePath, but it's named PathList. 
(It's a bit unfortunate that this code snippet has bounced around as 
patches without a definite name.) I assume you are going to push this 
through a hotspot forest. If the PathList patch reaches the hotspot repo 
before this, please remove the CreatePath from MakeBase, and change the 
calls to CreatePath to PathList instead. (I could only find such calls 
in hotspot/make/gensrc/Gensrc-java.base.gmk). If this patch goes in 
before that, we'll need to give a heads-up to the integrator about this 
conflict.


Another potential coming merge conflict relates to ListPathsSafely in 
Gensrc-java.base.gmk. There is currenlty a review out from Erik which 
modifies the API for ListPathsSafely. If/when it goes in, the call to 
ListPathsSafely in Gensrc-java.base.gmk will need to be modified (Erik 
can give advice on how). Depending on timing, this too might hit the 
integrator rather than your push.


/Magnus

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-16 Thread Christian Thalinger 

> On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2015-09-14 09:24, Christian Thalinger wrote:
>> The JEP itself can be found here:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8062493 
>> 
>> 
>> Here are the webrevs:
>> 
>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 
>> 
>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 
>> 
>> 
>> The change has already undergone a few iterations of internal review by 
>> different people of different teams.  Most comments and suggestions were 
>> handled accordingly.  Although there is one open item we agreed we will 
>> address after the integration of JEP 243 and that work is captured in RFE:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8134994 
>> 
>> 
>> SQE is still working on the tests and all test tasks can be seen as 
>> sub-tasks of the JEP.
>> 
>> The integration will happen under the bug number:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8136421 
>> 
>> 
> Hi Christian,
> 
> (Adding build-dev since this review includes some major build changes.)
> 
> The makefile changes looks good in general to me. I have not reviewed the 
> source code changes.
> 
> Some comments:
> 
> * modules.xml:
> Make sure you get sign-off from a Jigsaw developer for modifying this file.

I’ve asked Alan to take a look.

> 
> * hotspot/make/linux/makefiles/gcc.make:
> Seems unfortunate to have to disable a new warning 
> (undefined-bool-conversion) for newly incorporated code. Is it not possible 
> to fix the code emitting this warning instead?

We had this question before.  It’s not obvious because you can’t see it in the 
regular diff views but this is under:

ifeq ($(USE_CLANG), true)

> 
> * make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
> I see a coming merge conflict. In jdk9/dev, there is now a new function that 
> performs the same function as CreatePath, but it's named PathList. (It's a 
> bit unfortunate that this code snippet has bounced around as patches without 
> a definite name.) I assume you are going to push this through a hotspot 
> forest. If the PathList patch reaches the hotspot repo before this, please 
> remove the CreatePath from MakeBase, and change the calls to CreatePath to 
> PathList instead. (I could only find such calls in 
> hotspot/make/gensrc/Gensrc-java.base.gmk). If this patch goes in before that, 
> we'll need to give a heads-up to the integrator about this conflict.

Thanks for the heads up.

> 
> Another potential coming merge conflict relates to ListPathsSafely in 
> Gensrc-java.base.gmk. There is currenlty a review out from Erik which 
> modifies the API for ListPathsSafely. If/when it goes in, the call to 
> ListPathsSafely in Gensrc-java.base.gmk will need to be modified (Erik can 
> give advice on how). Depending on timing, this too might hit the integrator 
> rather than your push.

Ok, thanks.

> 
> /Magnus

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-16 Thread Alan Bateman 



On 16/09/2015 13:57, Magnus Ihse Bursie wrote:

:

* modules.xml:
Make sure you get sign-off from a Jigsaw developer for modifying this 
file.

The update to modules.xml looks fine.

-Alan.

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-16 Thread Magnus Ihse Bursie 

On 2015-09-16 18:52, Christian Thalinger wrote:

On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie  
wrote:

On 2015-09-14 09:24, Christian Thalinger wrote:

The JEP itself can be found here:

https://bugs.openjdk.java.net/browse/JDK-8062493 


Here are the webrevs:

http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 

http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 


The change has already undergone a few iterations of internal review by 
different people of different teams.  Most comments and suggestions were 
handled accordingly.  Although there is one open item we agreed we will address 
after the integration of JEP 243 and that work is captured in RFE:

https://bugs.openjdk.java.net/browse/JDK-8134994 


SQE is still working on the tests and all test tasks can be seen as sub-tasks 
of the JEP.

The integration will happen under the bug number:

https://bugs.openjdk.java.net/browse/JDK-8136421 



Hi Christian,

(Adding build-dev since this review includes some major build changes.)

The makefile changes looks good in general to me. I have not reviewed the 
source code changes.

Some comments:

* modules.xml:
Make sure you get sign-off from a Jigsaw developer for modifying this file.

I’ve asked Alan to take a look.


* hotspot/make/linux/makefiles/gcc.make:
Seems unfortunate to have to disable a new warning (undefined-bool-conversion) 
for newly incorporated code. Is it not possible to fix the code emitting this 
warning instead?

We had this question before.  It’s not obvious because you can’t see it in the 
regular diff views but this is under:

ifeq ($(USE_CLANG), true)


I'm not sure I understand why that's relevant..? Isn't it just as 
important to try to submit warning-free code when compiling with clang 
as with any other compiler? Or is clang just being anal about the code 
in question?


/Magnus




* make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
I see a coming merge conflict. In jdk9/dev, there is now a new function that 
performs the same function as CreatePath, but it's named PathList. (It's a bit 
unfortunate that this code snippet has bounced around as patches without a 
definite name.) I assume you are going to push this through a hotspot forest. 
If the PathList patch reaches the hotspot repo before this, please remove the 
CreatePath from MakeBase, and change the calls to CreatePath to PathList 
instead. (I could only find such calls in 
hotspot/make/gensrc/Gensrc-java.base.gmk). If this patch goes in before that, 
we'll need to give a heads-up to the integrator about this conflict.

Thanks for the heads up.


Another potential coming merge conflict relates to ListPathsSafely in 
Gensrc-java.base.gmk. There is currenlty a review out from Erik which modifies 
the API for ListPathsSafely. If/when it goes in, the call to ListPathsSafely in 
Gensrc-java.base.gmk will need to be modified (Erik can give advice on how). 
Depending on timing, this too might hit the integrator rather than your push.

Ok, thanks.


/Magnus

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-16 Thread Christian Thalinger 

> On Sep 16, 2015, at 10:11 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2015-09-16 18:52, Christian Thalinger wrote:
>>> On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie 
>>>  wrote:
>>> 
>>> On 2015-09-14 09:24, Christian Thalinger wrote:
 The JEP itself can be found here:
 
 https://bugs.openjdk.java.net/browse/JDK-8062493 
 
 
 Here are the webrevs:
 
 http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 
 
 http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 
 
 
 The change has already undergone a few iterations of internal review by 
 different people of different teams.  Most comments and suggestions were 
 handled accordingly. Although there is one open item we agreed we will 
 address after the integration of JEP 243 and that work is captured in RFE:
 
 https://bugs.openjdk.java.net/browse/JDK-8134994 
 
 
 SQE is still working on the tests and all test tasks can be seen as 
 sub-tasks of the JEP.
 
 The integration will happen under the bug number:
 
 https://bugs.openjdk.java.net/browse/JDK-8136421 
 
 
>>> Hi Christian,
>>> 
>>> (Adding build-dev since this review includes some major build changes.)
>>> 
>>> The makefile changes looks good in general to me. I have not reviewed the 
>>> source code changes.
>>> 
>>> Some comments:
>>> 
>>> * modules.xml:
>>> Make sure you get sign-off from a Jigsaw developer for modifying this file.
>> I’ve asked Alan to take a look.
>> 
>>> * hotspot/make/linux/makefiles/gcc.make:
>>> Seems unfortunate to have to disable a new warning 
>>> (undefined-bool-conversion) for newly incorporated code. Is it not possible 
>>> to fix the code emitting this warning instead?
>> We had this question before.  It’s not obvious because you can’t see it in 
>> the regular diff views but this is under:
>> 
>> ifeq ($(USE_CLANG), true)
> 
> I'm not sure I understand why that's relevant..? Isn't it just as important 
> to try to submit warning-free code when compiling with clang as with any 
> other compiler? Or is clang just being anal about the code in question?

I don’t have a Clang compiler at hand but Clang is anal about everything.  Do 
you want that suppression to be removed?

> 
> /Magnus
> 
>> 
>>> * make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
>>> I see a coming merge conflict. In jdk9/dev, there is now a new function 
>>> that performs the same function as CreatePath, but it's named PathList. 
>>> (It's a bit unfortunate that this code snippet has bounced around as 
>>> patches without a definite name.) I assume you are going to push this 
>>> through a hotspot forest. If the PathList patch reaches the hotspot repo 
>>> before this, please remove the CreatePath from MakeBase, and change the 
>>> calls to CreatePath to PathList instead. (I could only find such calls in 
>>> hotspot/make/gensrc/Gensrc-java.base.gmk). If this patch goes in before 
>>> that, we'll need to give a heads-up to the integrator about this conflict.
>> Thanks for the heads up.
>> 
>>> Another potential coming merge conflict relates to ListPathsSafely in 
>>> Gensrc-java.base.gmk. There is currenlty a review out from Erik which 
>>> modifies the API for ListPathsSafely. If/when it goes in, the call to 
>>> ListPathsSafely in Gensrc-java.base.gmk will need to be modified (Erik can 
>>> give advice on how). Depending on timing, this too might hit the integrator 
>>> rather than your push.
>> Ok, thanks.
>> 
>>> /Magnus

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-16 Thread Christian Thalinger 

> On Sep 16, 2015, at 6:52 AM, Christian Thalinger 
>  wrote:
> 
> 
>> On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie 
>>  wrote:
>> 
>> On 2015-09-14 09:24, Christian Thalinger wrote:
>>> The JEP itself can be found here:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8062493 
>>> 
>>> 
>>> Here are the webrevs:
>>> 
>>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 
>>> 
>>> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 
>>> 
>>> 
>>> The change has already undergone a few iterations of internal review by 
>>> different people of different teams.  Most comments and suggestions were 
>>> handled accordingly.  Although there is one open item we agreed we will 
>>> address after the integration of JEP 243 and that work is captured in RFE:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8134994 
>>> 
>>> 
>>> SQE is still working on the tests and all test tasks can be seen as 
>>> sub-tasks of the JEP.
>>> 
>>> The integration will happen under the bug number:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8136421 
>>> 
>>> 
>> Hi Christian,
>> 
>> (Adding build-dev since this review includes some major build changes.)
>> 
>> The makefile changes looks good in general to me. I have not reviewed the 
>> source code changes.
>> 
>> Some comments:
>> 
>> * modules.xml:
>> Make sure you get sign-off from a Jigsaw developer for modifying this file.
> 
> I’ve asked Alan to take a look.
> 
>> 
>> * hotspot/make/linux/makefiles/gcc.make:
>> Seems unfortunate to have to disable a new warning 
>> (undefined-bool-conversion) for newly incorporated code. Is it not possible 
>> to fix the code emitting this warning instead?
> 
> We had this question before.  It’s not obvious because you can’t see it in 
> the regular diff views but this is under:
> 
> ifeq ($(USE_CLANG), true)
> 
>> 
>> * make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
>> I see a coming merge conflict. In jdk9/dev, there is now a new function that 
>> performs the same function as CreatePath, but it's named PathList. (It's a 
>> bit unfortunate that this code snippet has bounced around as patches without 
>> a definite name.) I assume you are going to push this through a hotspot 
>> forest. If the PathList patch reaches the hotspot repo before this, please 
>> remove the CreatePath from MakeBase, and change the calls to CreatePath to 
>> PathList instead. (I could only find such calls in 
>> hotspot/make/gensrc/Gensrc-java.base.gmk). If this patch goes in before 
>> that, we'll need to give a heads-up to the integrator about this conflict.
> 
> Thanks for the heads up.

Erik sent me a patch to avoid merge conflicts.  I’ve integrated two changesets:

http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab 

http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9 


> 
>> 
>> Another potential coming merge conflict relates to ListPathsSafely in 
>> Gensrc-java.base.gmk. There is currenlty a review out from Erik which 
>> modifies the API for ListPathsSafely. If/when it goes in, the call to 
>> ListPathsSafely in Gensrc-java.base.gmk will need to be modified (Erik can 
>> give advice on how). Depending on timing, this too might hit the integrator 
>> rather than your push.
> 
> Ok, thanks.
> 
>> 
>> /Magnus
>

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-16 Thread Vladimir Kozlov 

I updated top and hotspot webrev with latest (build) changes.

Vladimir

On 9/16/15 2:28 PM, Christian Thalinger wrote:



On Sep 16, 2015, at 6:52 AM, Christian Thalinger 
 wrote:



On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie  
wrote:

On 2015-09-14 09:24, Christian Thalinger wrote:

The JEP itself can be found here:

https://bugs.openjdk.java.net/browse/JDK-8062493 


Here are the webrevs:

http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 

http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 


The change has already undergone a few iterations of internal review by 
different people of different teams.  Most comments and suggestions were 
handled accordingly.  Although there is one open item we agreed we will address 
after the integration of JEP 243 and that work is captured in RFE:

https://bugs.openjdk.java.net/browse/JDK-8134994 


SQE is still working on the tests and all test tasks can be seen as sub-tasks 
of the JEP.

The integration will happen under the bug number:

https://bugs.openjdk.java.net/browse/JDK-8136421 



Hi Christian,

(Adding build-dev since this review includes some major build changes.)

The makefile changes looks good in general to me. I have not reviewed the 
source code changes.

Some comments:

* modules.xml:
Make sure you get sign-off from a Jigsaw developer for modifying this file.


I’ve asked Alan to take a look.



* hotspot/make/linux/makefiles/gcc.make:
Seems unfortunate to have to disable a new warning (undefined-bool-conversion) 
for newly incorporated code. Is it not possible to fix the code emitting this 
warning instead?


We had this question before.  It’s not obvious because you can’t see it in the 
regular diff views but this is under:

ifeq ($(USE_CLANG), true)



* make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
I see a coming merge conflict. In jdk9/dev, there is now a new function that 
performs the same function as CreatePath, but it's named PathList. (It's a bit 
unfortunate that this code snippet has bounced around as patches without a 
definite name.) I assume you are going to push this through a hotspot forest. 
If the PathList patch reaches the hotspot repo before this, please remove the 
CreatePath from MakeBase, and change the calls to CreatePath to PathList 
instead. (I could only find such calls in 
hotspot/make/gensrc/Gensrc-java.base.gmk). If this patch goes in before that, 
we'll need to give a heads-up to the integrator about this conflict.


Thanks for the heads up.


Erik sent me a patch to avoid merge conflicts.  I’ve integrated two changesets:

http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab 

http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9 






Another potential coming merge conflict relates to ListPathsSafely in 
Gensrc-java.base.gmk. There is currenlty a review out from Erik which modifies 
the API for ListPathsSafely. If/when it goes in, the call to ListPathsSafely in 
Gensrc-java.base.gmk will need to be modified (Erik can give advice on how). 
Depending on timing, this too might hit the integrator rather than your push.


Ok, thanks.



/Magnus

Re: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface

2015-09-16 Thread Christian Thalinger 
I would like to add this change:

diff -r 2134e08cc132 src/share/vm/utilities/vmError.cpp
--- a/src/share/vm/utilities/vmError.cppWed Sep 16 14:28:33 2015 -1000
+++ b/src/share/vm/utilities/vmError.cppWed Sep 16 15:04:02 2015 -1000
@@ -517,6 +517,10 @@ void VMError::report(outputStream* st) {
Abstract_VM_Version::vm_release(),
Abstract_VM_Version::vm_info_string(),
TieredCompilation ? ", tiered" : "",
+#if INCLUDE_JVMCI
+   EnableJVMCI ? ", jvmci" : "",
+   UseJVMCICompiler ? ", jvmci compiler" : "",
+#endif
UseCompressedOops ? ", compressed oops" : "",
gc_mode(),
Abstract_VM_Version::vm_platform_string()

It would be useful to see in the crash logs if this experimental feature was 
turned on.

> On Sep 16, 2015, at 12:43 PM, Vladimir Kozlov  
> wrote:
> 
> I updated top and hotspot webrev with latest (build) changes.
> 
> Vladimir
> 
> On 9/16/15 2:28 PM, Christian Thalinger wrote:
>> 
>>> On Sep 16, 2015, at 6:52 AM, Christian Thalinger 
>>>  wrote:
>>> 
>>> 
 On Sep 16, 2015, at 2:57 AM, Magnus Ihse Bursie 
  wrote:
 
 On 2015-09-14 09:24, Christian Thalinger wrote:
> The JEP itself can be found here:
> 
> https://bugs.openjdk.java.net/browse/JDK-8062493 
> 
> 
> Here are the webrevs:
> 
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.top/ 
> 
> http://cr.openjdk.java.net/~kvn/JVMCI/webrev.hotspot/ 
> 
> 
> The change has already undergone a few iterations of internal review by 
> different people of different teams.  Most comments and suggestions were 
> handled accordingly.  Although there is one open item we agreed we will 
> address after the integration of JEP 243 and that work is captured in RFE:
> 
> https://bugs.openjdk.java.net/browse/JDK-8134994 
> 
> 
> SQE is still working on the tests and all test tasks can be seen as 
> sub-tasks of the JEP.
> 
> The integration will happen under the bug number:
> 
> https://bugs.openjdk.java.net/browse/JDK-8136421 
> 
> 
 Hi Christian,
 
 (Adding build-dev since this review includes some major build changes.)
 
 The makefile changes looks good in general to me. I have not reviewed the 
 source code changes.
 
 Some comments:
 
 * modules.xml:
 Make sure you get sign-off from a Jigsaw developer for modifying this file.
>>> 
>>> I’ve asked Alan to take a look.
>>> 
 
 * hotspot/make/linux/makefiles/gcc.make:
 Seems unfortunate to have to disable a new warning 
 (undefined-bool-conversion) for newly incorporated code. Is it not 
 possible to fix the code emitting this warning instead?
>>> 
>>> We had this question before.  It’s not obvious because you can’t see it in 
>>> the regular diff views but this is under:
>>> 
>>> ifeq ($(USE_CLANG), true)
>>> 
 
 * make/common/MakeBase.gmk and hotspot/make/gensrc/Gensrc-java.base.gmk:
 I see a coming merge conflict. In jdk9/dev, there is now a new function 
 that performs the same function as CreatePath, but it's named PathList. 
 (It's a bit unfortunate that this code snippet has bounced around as 
 patches without a definite name.) I assume you are going to push this 
 through a hotspot forest. If the PathList patch reaches the hotspot repo 
 before this, please remove the CreatePath from MakeBase, and change the 
 calls to CreatePath to PathList instead. (I could only find such calls in 
 hotspot/make/gensrc/Gensrc-java.base.gmk). If this patch goes in before 
 that, we'll need to give a heads-up to the integrator about this conflict.
>>> 
>>> Thanks for the heads up.
>> 
>> Erik sent me a patch to avoid merge conflicts.  I’ve integrated two 
>> changesets:
>> 
>> http://hg.openjdk.java.net/graal/graal-jvmci-9/rev/ddedccc5c0ab 
>> >  >
>> http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/fee6b89199c9 
>> >  >
>> 
>>> 
 
 Another potential coming merge conflict relates to ListPathsSafely in 
 Gensrc-java.base.gmk. There is currenlty a review out