Re: RFR: 8275874: [JVMCI] use volatile accessors for aligned reads in c2v_readFieldValue

[Doug Simon]

On Tue, 26 Oct 2021 05:20:22 GMT, David Holmes  wrote:

> Isn't the title of this issue expressed incorrectly?

Yes - thanks for pointing that out.

-

PR: https://git.openjdk.java.net/jdk/pull/6109

Re: RFR: 8275874: [JVMCI] use volatile accessors for all unaligned reads in c2v_readFieldValue

[Doug Simon]

On Mon, 25 Oct 2021 14:33:27 GMT, Doug Simon  wrote:

> [JDK-8275645](https://bugs.openjdk.java.net/browse/JDK-8275645) resulted in 
> loosing single-copy atomicity for reads in `c2v_readFieldValue`. This PR 
> fixes that by using `_field_acquire` accessors for all aligned reads 
> and only using `_field` accessors for unaligned reads.

@shipilev , it would be great if you could review this.

-

PR: https://git.openjdk.java.net/jdk/pull/6109

RFR: 8275874: [JVMCI] use volatile accessors for all unaligned reads in c2v_readFieldValue

[Doug Simon]

[JDK-8275645](https://bugs.openjdk.java.net/browse/JDK-8275645), resulted in 
the loose of single-copy atomicity for reads in c2v_readFieldValue. This PR 
fixes that by using the `_field_acquire` accessors for all aligned reads 
in c2v_readFieldValue and only using the `_field` accessors for unaligned 
reads.

-

Commit messages:
 - use _field_acquire for aligned reads in c2v_readFieldValue

Changes: https://git.openjdk.java.net/jdk/pull/6109/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6109=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8275874
  Stats: 30 lines in 1 file changed: 2 ins; 18 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6109.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6109/head:pull/6109

PR: https://git.openjdk.java.net/jdk/pull/6109

Re: RFR: 8267112: JVMCI compiler modules should be kept upgradable

[Doug Simon]

On Thu, 13 May 2021 16:37:38 GMT, Vladimir Kozlov  wrote:

> [JDK-8264806](https://bugs.openjdk.java.net/browse/JDK-8264806) changes 
> removed sources and also removed JVMCI compiler from list of upgradable 
> modules. JVMCI compiler modules should be upgradable in JDK to work with 
> GraalVM. 
> 
> Make these modules upgradable again and empty by leaving only reference to 
> JVMCI (jdk.internal.vm.ci) module. It does not restore sources - only 
> `module-info.java` files are kept.
> 
> Note, we continue discussion about 
> [JDK-8265091](https://bugs.openjdk.java.net/browse/JDK-8265091): "Use Module 
> API to export JVMCI packages at runtime" to see if we can remove these 
> `module-info.java` files.
> 
> Changes were proposed by @dougxc after testing 
> [JDK-8264806](https://bugs.openjdk.java.net/browse/JDK-8264806) changes with 
> GraalVM.
> I restored related code in some tests for them to pass.
> 
> Testing: full tier1-tier3.

Looks good. My only suggestion would have been to avoid mentioning "Graal" in 
the PR title and description. This is really about making 
`jdk.internal.vm.compiler` a placeholder module for any JVMCI based compiler, 
of which Graal is one example.

-

PR: https://git.openjdk.java.net/jdk/pull/4014

Re: RFR: 8264806: Remove the experimental JIT compiler [v2]

[Doug Simon]

On Tue, 27 Apr 2021 19:07:45 GMT, Igor Ignatyev  wrote:

> > I guess this should really be named `isJVMCICompilerEnabled` now and the 
> > `vm.graal.enabled` predicate renamed to `vm.jvmcicompiler.enabled` but 
> > maybe that's too big a change (or can be done later).
> 
> @dougxc, I don't think that we should (or even can) rename it to 
> `vm.jvmcicompiler.enabled`. although the value of the property is indeed 
> `true` whenever a jvmci compiler is enabled, it is used to filter out tests 
> that are incompatible with a particular compiler -- graal. so what we can do 
> is to change `requires/VMProps.java` to set `vm.graal.enabled` only if the 
> jvmci compiler is indeed graal, but on the other hand, we haven't heard 
> anyone complaining that the test coverage for their jvmci compiler has been 
> reduced, so I don't see much of the benefit here.
> 
> -- Igor

Yes, we should just it as is until a second JVMCI compiler shows up.

-

PR: https://git.openjdk.java.net/jdk/pull/3421

Integrated: 8265403: consolidate definition of CPU features

[Doug Simon]

On Sat, 17 Apr 2021 20:18:31 GMT, Doug Simon  wrote:

> While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) 
> to Graal, I noticed that new CPU features were defined for x86 and AArch64 
> without being exposed via JVMCI. To avoid this problem in future, this PR 
> updates x86 and AArch64 to define CPU features with a single macro that is 
> used to generate enum declarations as well as vmstructs entries.
> 
> In addition, the JVMCI API is updated to exposes the new CPU feature 
> constants and now has a check that ensure these constants are in sync with 
> the underlying macro definition.

This pull request has now been integrated.

Changeset: 5aed446e
Author:Doug Simon 
URL:   https://git.openjdk.java.net/jdk/commit/5aed446e
Stats: 493 lines in 15 files changed: 146 ins; 235 del; 112 mod

8265403: consolidate definition of CPU features

Reviewed-by: kvn, iklam

-

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8265403: consolidate definition of CPU features [v4]

[Doug Simon]

On Mon, 19 Apr 2021 20:01:46 GMT, Vladimir Kozlov  wrote:

>> Doug Simon has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - updated date in copyright
>>  - added blank lines after macros
>
> You need review from Runtime group too.

Thanks for the reviews @vnkozlov and @iklam.

-

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8265403: consolidate definition of CPU features [v4]

[Doug Simon]

> While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) 
> to Graal, I noticed that new CPU features were defined for x86 and AArch64 
> without being exposed via JVMCI. To avoid this problem in future, this PR 
> updates x86 and AArch64 to define CPU features with a single macro that is 
> used to generate enum declarations as well as vmstructs entries.
> 
> In addition, the JVMCI API is updated to exposes the new CPU feature 
> constants and now has a check that ensure these constants are in sync with 
> the underlying macro definition.

Doug Simon has updated the pull request incrementally with two additional 
commits since the last revision:

 - updated date in copyright
 - added blank lines after macros

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3558/files
  - new: https://git.openjdk.java.net/jdk/pull/3558/files/124019d7..c6be1437

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3558=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3558=02-03

  Stats: 11 lines in 10 files changed: 2 ins; 0 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3558.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3558/head:pull/3558

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8265403: consolidate definition of CPU features [v3]

[Doug Simon]

On Mon, 19 Apr 2021 19:00:09 GMT, Vladimir Kozlov  wrote:

>> Doug Simon has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR. The pull request contains one 
>> new commit since the last revision:
>> 
>>   avoid use of a lambda in JVMCI initialization
>
> src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 753:
> 
>> 751: 
>> 752: #define DECLARE_INT_CPU_FEATURE_CONSTANT(id, name, bit) 
>> GENERATE_VM_INT_CONSTANT_ENTRY(VM_Version::CPU_##id)
>> 753: #define VM_INT_CPU_FEATURE_CONSTANTS 
>> CPU_FEATURE_FLAGS(DECLARE_INT_CPU_FEATURE_CONSTANT)
> 
> Missing `#undef DECLARE_INT_CPU_FEATURE_CONSTANT`.

No, it must stay defined up to the point `VM_INT_CPU_FEATURE_CONSTANTS` is 
used. Since this is a `.cpp` file, it's ok to leave it defined.

> src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 769:
> 
>> 767: 
>> 768: #define DECLARE_LONG_CPU_FEATURE_CONSTANT(id, name, bit) 
>> GENERATE_VM_LONG_CONSTANT_ENTRY(VM_Version::CPU_##id)
>> 769: #define VM_LONG_CPU_FEATURE_CONSTANTS 
>> CPU_FEATURE_FLAGS(DECLARE_LONG_CPU_FEATURE_CONSTANT)
> 
> Missing `#undef DECLARE_LONG_CPU_FEATURE_CONSTANT`.

So comment as for `DECLARE_INT_CPU_FEATURE_CONSTANT`.

> src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotJVMCIBackendFactory.java
>  line 66:
> 
>> 64: long bitMask = e.getValue();
>> 65: String key = e.getKey();
>> 66: if (key.startsWith("VM_Version::CPU_")) {
> 
> As I understand this code, it goes over `constants` values passed from VM and 
> Trying to map them to `enumType`. It catches cases when a value is missing in 
> `enumType`. What about case when `enumType` has `extra` value which is not 
> defined in `constants`?

We could warn about that but cannot remove it without breaking backwards 
capability for JVMCI wrt Graal. Such a deleted capability will simply be seen 
as "not supported" by Graal.

-

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8265403: consolidate definition of CPU features [v3]

[Doug Simon]

On Mon, 19 Apr 2021 18:58:46 GMT, Vladimir Kozlov  wrote:

>> Doug Simon has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR. The pull request contains one 
>> new commit since the last revision:
>> 
>>   avoid use of a lambda in JVMCI initialization
>
> src/hotspot/cpu/x86/vmStructs_x86.hpp line 45:
> 
>> 43: 
>> 44: #define DECLARE_LONG_CPU_FEATURE_CONSTANT(id, name, bit) 
>> GENERATE_VM_LONG_CONSTANT_ENTRY(VM_Version::CPU_##id)
>> 45: #define VM_LONG_CPU_FEATURE_CONSTANTS 
>> CPU_FEATURE_FLAGS(DECLARE_LONG_CPU_FEATURE_CONSTANT)
> 
> Do we need to keep `VM_LONG_CONSTANTS_CPU` after you removed its body here 
> and in vmStructs_jvmci.cpp?
> 
> What about `VM_INT_CONSTANTS_CPU` here? vmStructs_jvmci.cpp duplicates it.

`vmStructs.cpp` and `vmStructs_jvmci.cpp` are disjoint. This file (i.e. 
`vmStructs_x86.hpp`) is only used by `vmStructs.cpp`.
`vmStructs.cpp` expects all macros such as `VM_LONG_CONSTANTS_CPU` to be 
defined.
`vmStructs_jvmci.cpp` will provide a dummy definition for missing macros.

-

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8265403: consolidate definition of CPU features [v3]

[Doug Simon]

On Mon, 19 Apr 2021 18:57:37 GMT, Vladimir Kozlov  wrote:

>> Doug Simon has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR. The pull request contains one 
>> new commit since the last revision:
>> 
>>   avoid use of a lambda in JVMCI initialization
>
> src/hotspot/cpu/aarch64/vm_version_aarch64.hpp line 118:
> 
>> 116: decl(STXR_PREFETCH, "stxr_prefetch", 29)  \
>> 117: decl(A53MAC,"a53mac",30)
>> 118: #define DECLARE_CPU_FEATURE_FLAG(id, name, bit) CPU_##id = (1 << bit),
> 
> Add empty line before to separate `CPU_FEATURE_FLAGS` macro.

Ok.

-

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8252600: [JVMCI] remove mx configuration

[Doug Simon]

On Sun, 18 Apr 2021 07:32:07 GMT, Alan Bateman  wrote:

>> This PR removes the mx configuration files in the JDK as they do not really 
>> belong here. Instead, I've updated and moved them to 
>> https://github.com/dougxc/mx_jdk.
>
> Are you sure it make sense to have this dev config in the openjdk/jdk repo? I 
> would think this is something for the downstream Graal repos.

Thanks @AlanBateman , @erikj79 and @vnkozlov for the reviews.

-

PR: https://git.openjdk.java.net/jdk/pull/3559

Integrated: 8252600: [JVMCI] remove mx configuration

[Doug Simon]

On Sat, 17 Apr 2021 20:37:08 GMT, Doug Simon  wrote:

> This PR removes the mx configuration files in the JDK as they do not really 
> belong here. Instead, I've updated and moved them to 
> https://github.com/dougxc/mx_jdk.

This pull request has now been integrated.

Changeset: 54cb388d
Author:Doug Simon 
URL:   https://git.openjdk.java.net/jdk/commit/54cb388d
Stats: 1346 lines in 7 files changed: 0 ins; 1346 del; 0 mod

8252600: [JVMCI] remove mx configuration

Reviewed-by: erikj, kvn

-

PR: https://git.openjdk.java.net/jdk/pull/3559

Re: RFR: 8265403: consolidate definition of CPU features [v3]

[Doug Simon]

> While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) 
> to Graal, I noticed that new CPU features were defined for x86 and AArch64 
> without being exposed via JVMCI. To avoid this problem in future, this PR 
> updates x86 and AArch64 to define CPU features with a single macro that is 
> used to generate enum declarations as well as vmstructs entries.
> 
> In addition, the JVMCI API is updated to exposes the new CPU feature 
> constants and now has a check that ensure these constants are in sync with 
> the underlying macro definition.

Doug Simon has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  avoid use of a lambda in JVMCI initialization

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3558/files
  - new: https://git.openjdk.java.net/jdk/pull/3558/files/bdf9eefb..124019d7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3558=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3558=01-02

  Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3558.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3558/head:pull/3558

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8265403: consolidate definition of CPU features [v2]

[Doug Simon]

> While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) 
> to Graal, I noticed that new CPU features were defined for x86 and AArch64 
> without being exposed via JVMCI. To avoid this problem in future, this PR 
> updates x86 and AArch64 to define CPU features with a single macro that is 
> used to generate enum declarations as well as vmstructs entries.
> 
> In addition, the JVMCI API is updated to exposes the new CPU feature 
> constants and now has a check that ensure these constants are in sync with 
> the underlying macro definition.

Doug Simon has updated the pull request incrementally with one additional 
commit since the last revision:

  avoid use of a lambda in JVMCI initialization

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3558/files
  - new: https://git.openjdk.java.net/jdk/pull/3558/files/b69675f4..bdf9eefb

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3558=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3558=00-01

  Stats: 23 lines in 3 files changed: 5 ins; 9 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3558.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3558/head:pull/3558

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8252600: remove mx configuration [v2]

[Doug Simon]

On Sun, 18 Apr 2021 20:17:14 GMT, Doug Simon  wrote:

>> This PR removes the mx configuration files in the JDK as they do not really 
>> belong here. Instead, I've updated and moved them to 
>> https://github.com/dougxc/mx_jdk.
>
> Doug Simon has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   8252600: [JVMCI] remove mx configuration

On second thoughts, I concur with you Alan and am thus using this PR to remove 
the mx configuration files.

-

PR: https://git.openjdk.java.net/jdk/pull/3559

Re: RFR: 8252600: remove mx configuration [v2]

[Doug Simon]

> This PR updates the configuration files used to develop the JVMCI Java and 
> C++ sources with mx and Eclipse.

Doug Simon has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  8252600: [JVMCI] remove mx configuration

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3559/files
  - new: https://git.openjdk.java.net/jdk/pull/3559/files/7026605f..7c18d2d9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3559=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3559=00-01

  Stats: 754 lines in 8 files changed: 0 ins; 754 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3559.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3559/head:pull/3559

PR: https://git.openjdk.java.net/jdk/pull/3559

RFR: 8265403: consolidate definition of CPU features

[Doug Simon]

While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) 
to Graal, I noticed that new CPU features were defined for x86 and AArch64 
without being exposed via JVMCI. To avoid this problem in future, this PR 
updates x86 and AArch64 to define CPU features with a single macro that is used 
to generate enum declarations as well as vmstructs entries.

In addition, the JVMCI API is updated to exposes the new CPU feature constants 
and now has a check that ensure these constants are in sync with the underlying 
macro definition.

-

Commit messages:
 - 8265403: consolidate definition of CPU features

Changes: https://git.openjdk.java.net/jdk/pull/3558/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3558=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265403
  Stats: 481 lines in 15 files changed: 144 ins; 230 del; 107 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3558.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3558/head:pull/3558

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8265403: consolidate definition of CPU features

[Doug Simon]

On Sat, 17 Apr 2021 20:18:31 GMT, Doug Simon  wrote:

> While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) 
> to Graal, I noticed that new CPU features were defined for x86 and AArch64 
> without being exposed via JVMCI. To avoid this problem in future, this PR 
> updates x86 and AArch64 to define CPU features with a single macro that is 
> used to generate enum declarations as well as vmstructs entries.
> 
> In addition, the JVMCI API is updated to exposes the new CPU feature 
> constants and now has a check that ensure these constants are in sync with 
> the underlying macro definition.

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 198:

> 196:   sprintf(buf, "0x%02x:0x%x:0x%03x:%d", _cpu, _variant, _model, 
> _revision);
> 197:   if (_model2) sprintf(buf+strlen(buf), "(0x%03x)", _model2);
> 198: #define ADD_FEATURE_IF_SUPPORTED(id, name, bit) if (_features & 
> CPU_##id) strcat(buf, ", " name);

I'm not sure why only some of the supported AArch64 CPU features were being 
added to `_features_string` but I assume there's no harm in adding them all.

src/hotspot/cpu/x86/vm_version_x86.hpp line 382:

> 380:   static const char* _features_names[];
> 381: 
> 382:   // NB! When adding new CPU feature detection consider updating 
> vmStructs_x86.hpp, vmStructs_jvmci.hpp, and 
> VM_Version::get_processor_features().

No need for this comment any more as the derivative declarations are now 
automatically kept up to date.

src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot.amd64/src/jdk/vm/ci/hotspot/amd64/AMD64HotSpotJVMCIBackendFactory.java
 line 57:

> 55: 
> 56: Map constants = config.getStore().getConstants();
> 57: Function nameToFeature = name -> 
> name.equals("3DNOW_PREFETCH") ? CPUFeature.AMD_3DNOW_PREFETCH : 
> CPUFeature.valueOf(name);

The `AMD_3DNOW_PREFETCH` enum constant has to keep its old name to preserve 
backward compatibility.

-

PR: https://git.openjdk.java.net/jdk/pull/3558

Re: RFR: 8252600: [JVMCI] update JVMCI code style and mx configuration

[Doug Simon]

On Sat, 17 Apr 2021 20:37:08 GMT, Doug Simon  wrote:

> This PR updates the configuration files used to develop the JVMCI Java and 
> C++ sources with mx and Eclipse.

Until we have downstream repos for JDK 17, it's very handy to have this config 
in the JDK. I've tried to keep it only in JVMCI related directories so as to 
not perturb non-JVMCI sources. However, if the consensus is that this config 
does not belong in the JDK at all, I will repurpose this PR to remove all such 
config as it's current broken as is.

-

PR: https://git.openjdk.java.net/jdk/pull/3559

RFR: 8252600: [JVMCI] update JVMCI code style and mx configuration

[Doug Simon]

This PR updates the configuration files used to develop the JVMCI Java and C++ 
sources with mx and Eclipse.

-

Commit messages:
 - 8252600: [JVMCI] update JVMCI code style and mx configuration

Changes: https://git.openjdk.java.net/jdk/pull/3559/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3559=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252600
  Stats: 2096 lines in 14 files changed: 752 ins; 1344 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3559.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3559/head:pull/3559

PR: https://git.openjdk.java.net/jdk/pull/3559

Re: RFR: 8264806: Remove the experimental JIT compiler [v3]

[Doug Simon]

On Mon, 12 Apr 2021 22:10:06 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Java-based JIT compiler (Graal) from JDK:
>> 
>> - `jdk.internal.vm.compiler` — the Graal compiler 
>> - `jdk.internal.vm.compiler.management` — Graal's `MBean`
>> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests
>> 
>> Remove Graal related code in makefiles.
>> 
>> Note, next two `module-info.java` files are preserved so that the JVMCI 
>> module `jdk.internal.vm.ci` continues to build:
>> 
>> src/jdk.internal.vm.compiler/share/classes/module-info.java
>> src/jdk.internal.vm.compiler.management/share/classes/module-info.java
>> 
>> 
>> @AlanBateman suggested that we can avoid it by using Module API to export 
>> packages at runtime . It requires changes in GraalVM's JVMCI too so I will 
>> file followup RFE to implement it.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Restore Compiler::isGraalEnabled()

Approved.

-

PR: https://git.openjdk.java.net/jdk/pull/3421

Re: RFR: 8264806: Remove the experimental JIT compiler [v2]

[Doug Simon]

On Sun, 11 Apr 2021 10:25:47 GMT, Doug Simon  wrote:

>> Vladimir Kozlov has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains three additional 
>> commits since the last revision:
>> 
>>  - Restore Graal Builder image makefile
>>  - Merge latest changes from branch 'JDK-8264805' into JDK-8264806
>>  - 8264806: Remove the experimental JIT compiler
>
> We would definitely like to be able to continue testing of GraalVM with the 
> JDK set of jtreg tests. So keeping `Compiler::isGraalEnabled()` working like 
> it does today is important.

> @dougxc I restored Compiler::isGraalEnabled().

Thanks. I guess this should really be named `isJVMCICompilerEnabled` now and 
the `vm.graal.enabled` predicate renamed to `vm.jvmcicompiler.enabled` but 
maybe that's too big a change (or can be done later).

-

PR: https://git.openjdk.java.net/jdk/pull/3421

Re: RFR: 8264806: Remove the experimental JIT compiler [v2]

[Doug Simon]

On Sat, 10 Apr 2021 17:41:05 GMT, Vladimir Kozlov  wrote:

>> Marked as reviewed by iignatyev (Reviewer).
>
> Thank you, Igor. I filed https://bugs.openjdk.java.net/browse/JDK-8265032

We would definitely like to be able to continue testing of GraalVM with the JDK 
set of jtreg tests. So keeping `Compiler::isGraalEnabled()` working like it 
does today is important.

-

PR: https://git.openjdk.java.net/jdk/pull/3421

Integrated: 8252709: Enable JVMCI when building linux-aarch64 at Oracle

[Doug Simon]

On Mon, 22 Feb 2021 20:15:33 GMT, Doug Simon  wrote:

> To allow for better testing of JVMCI support on AArch64 in aid of producing a 
> reliable GraalVM release on this platform, it should be included by default.

This pull request has now been integrated.

Changeset: 29c72631
Author:    Doug Simon 
URL:   https://git.openjdk.java.net/jdk/commit/29c72631
Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod

8252709: Enable JVMCI when building linux-aarch64 at Oracle

Reviewed-by: kvn

-

PR: https://git.openjdk.java.net/jdk/pull/2677

Integrated: 8252709: Enable JVMCI when building linux-aarch64 at Oracle

[Doug Simon]

To allow for better testing of JVMCI support on AArch64 in aid of producing a 
reliable GraalVM release on this platform, it should be included by default.

-

Commit messages:
 - 8252709: Enable JVMCI when building linux-aarch64 at Oracle

Changes: https://git.openjdk.java.net/jdk/pull/2677/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2677=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252709
  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2677.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2677/head:pull/2677

PR: https://git.openjdk.java.net/jdk/pull/2677

Re: RFR: 8254827: JVMCI: Enable it for Windows+AArch64

[Doug Simon]

On Thu, 15 Oct 2020 15:00:47 GMT, Bernhard Urban-Forster  
wrote:

> Use r18 as allocatable register on Linux only.
> 
> A bootstrap works now (it has been crashing before due to r18 being 
> allocated):
> $
> ./windows-aarch64-server-fastdebug/bin/java.exe 
> -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI 
> -version
> Bootstrapping JVMCI. in 17990 ms (compiled 
> 3330 methods)
> openjdk version "16-internal" 2021-03-16
> OpenJDK Runtime Environment (fastdebug build 
> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk)
> OpenJDK 64-Bit Server VM (fastdebug build 
> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode)
> 
> Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well.

src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot.aarch64/src/jdk/vm/ci/hotspot/aarch64/AArch64HotSpotRegisterConfig.java
line 126:

> 124: public static final Register metaspaceMethodRegister = r12;
> 125:
> 126: public static final Register platformRegister = r18;

There should be a comment here as "platform register" is rather ambiguous.

src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot.aarch64/src/jdk/vm/ci/hotspot/aarch64/AArch64HotSpotRegisterConfig.java
line 133:

> 131: private static final RegisterArray reservedRegisters = new 
> RegisterArray(rscratch1, rscratch2, threadRegister,
> fp, lr, r31, zr, sp); 132:
> 133: private static RegisterArray initAllocatable(Architecture arch, 
> boolean reserveForHeapBase, boolean linuxOs) {

Instead of `linuxOs`, `canUsePlatformRegister` is a better name. The logic of 
which OS does what belongs more in
AArch64HotSpotJVMCIBackendFactory.

-

PR: https://git.openjdk.java.net/jdk/pull/685

Re: RFR(XS): 8244061: Disable jvmci/graal/aot when building linux-aarch64 at Oracle

[Doug Simon]

Ok, thanks for the clarification. I wasn’t sure if external parties had managed 
to set up a jib server but it sounds like that’s not so easy in practice.

-Doug

> On 29 Apr 2020, at 11:02, Magnus Ihse Bursie  
> wrote:
> 
> On 2020-04-29 10:45, Doug Simon wrote:
>> If I understand correctly, this disables building jvmci/graal/aot in *any* 
>> linux-aarch64 jib based build, not just those done at Oracle. Wouldn’t this 
>> be better done on the jib command line?
> While this is technically correct, the jib tool is only used at Oracle, so in 
> practice, this is the same thing.
> 
> If/when some other party of the community starts using the jib configuration 
> (instead of creating their own), that will be a relevant feedback.
> 
> /Magnus
>> 
>> -Doug
>> 
>>> On 29 Apr 2020, at 06:12, Mikael Vidstedt  
>>> wrote:
>>> 
>>> 
>>> Please review this small change which disables JVMCI, Graal, and AOT when 
>>> building linux-aarch64 at Oracle, for now.
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244061
>>> webrev: 
>>> http://cr.openjdk.java.net/~mikael/webrevs/8244061/webrev.00/open/webrev/
>>> 
>>> Cheers,
>>> Mikael
>>> 
> 

Re: RFR(XS): 8244061: Disable jvmci/graal/aot when building linux-aarch64 at Oracle

[Doug Simon]

If I understand correctly, this disables building jvmci/graal/aot in *any* 
linux-aarch64 jib based build, not just those done at Oracle. Wouldn’t this be 
better done on the jib command line?

-Doug

> On 29 Apr 2020, at 06:12, Mikael Vidstedt  wrote:
> 
> 
> Please review this small change which disables JVMCI, Graal, and AOT when 
> building linux-aarch64 at Oracle, for now.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8244061
> webrev: 
> http://cr.openjdk.java.net/~mikael/webrevs/8244061/webrev.00/open/webrev/
> 
> Cheers,
> Mikael
> 

Re: RFR(S) 8205078: [Graal] org.graalvm.compiler.core.test.VerifyDebugUsageTest fails with "Expected exception: VerificationError"

[Doug Simon]

> On 7 Aug 2018, at 08:07, Ekaterina Pavlova  
> wrote:
> 
> Vladimir, thanks for prompt review.
> 
> I can revert the changes and add -XDstringConcat=inline to JAVAC flags for 
> all tests.
> I did it only for org/graalvm/compiler/core/test/VerifyDebugUsageTest.java 
> because
> Doug was not sure if this should be applied to all tests. Here what he said: 
> "I can't say for sure since
> we've only ever compiled the tests with JDK 8 (or --release 8). I would only 
> apply it to this test for now.
> 
> Doug, what is your opinion on this?

Still the same. Tests should fail fast if they cannot handle Indy strings so I 
think dealing with it on a case-by-case basis is the right approach.

-Doug

> 
> On 8/6/18 2:17 PM, Vladimir Kozlov wrote:
>> Can we switch off Indy strings concat for all Graal unit tests? Based on 
>> what Doug said: "We don't support the indy string pattern in these tests".
>> Note, Graal itself (and JVMCI) are compiled without Indy strings concat:
>> http://hg.openjdk.java.net/jdk/jdk/file/c23f1e4910f3/make/CompileJavaModules.gmk#l425
>> Thanks,
>> Vladimir
>> On 8/6/18 1:42 PM, Ekaterina Pavlova wrote:
>>> Hi All,
>>> 
>>> please review the change which introduces additional make target in 
>>> JtregGraalUnit.gmk
>>> to compile org/graalvm/compiler/core/test/VerifyDebugUsageTest.java with 
>>> -XDstringConcat=inline.
>>> Otherwise the test fails. Also moved building of 
>>> jdk.vm.compiler.replacements.verifier.jar into separate target.
>>> 
>>>  JBS: https://bugs.openjdk.java.net/browse/JDK-8205078
>>>   webrev: http://cr.openjdk.java.net/~epavlova//8205078/webrev.00/index.html
>>> testing: tested by building on all platforms and running graal unit tests 
>>> on Graal supported platforms.
>>> 
>>> 
>>> thanks,
>>> -katya
>>> 
> 

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Doug Simon]

> On 22 Jun 2018, at 23:29, Doug Simon  wrote:
> 
> 
> 
>> On 22 Jun 2018, at 23:16, Ekaterina Pavlova  
>> wrote:
>> 
>> Erik, Doug,
>> 
>> thank you a lot for your reviews and advises.
>> I fixed everything what Erik has pointed out, please see my answers inlined.
>> As about moving more staff in 'updategraalinopenjdk' can we consider this as 
>> next step?
>> I am not quite familiar with 'updategraalinopenjdk' and didn't contribute 
>> into Graal ws yet,
>> so I will prefer to do this improvement/refactoring as part of separate JDK 
>> issue.
> 
> Sorry for not being clearer. I don't expect you to fix/enhance 
> updategraalinopenjdk. I was simply commenting on a possible solution to 
> Joel's reasonable request for having Graal sources and test layout more 
> normalized in the JDK code base.

Of course I meant Erik's request ;-)

-Doug

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Doug Simon]

> On 22 Jun 2018, at 23:16, Ekaterina Pavlova  
> wrote:
> 
> Erik, Doug,
> 
> thank you a lot for your reviews and advises.
> I fixed everything what Erik has pointed out, please see my answers inlined.
> As about moving more staff in 'updategraalinopenjdk' can we consider this as 
> next step?
> I am not quite familiar with 'updategraalinopenjdk' and didn't contribute 
> into Graal ws yet,
> so I will prefer to do this improvement/refactoring as part of separate JDK 
> issue.

Sorry for not being clearer. I don't expect you to fix/enhance 
updategraalinopenjdk. I was simply commenting on a possible solution to Joel's 
reasonable request for having Graal sources and test layout more normalized in 
the JDK code base.

-Doug

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Doug Simon]

> On 19 Jun 2018, at 23:00, Erik Joelsson  wrote:
> 
> Hello,
> 
> Please always include build-dev when reviewing build changes.
> 
> In general this looks pretty good but there are some details that need fixing.
> 
> (Aside from this particular review, I must state my reservation against all 
> the special treatment graal is enjoying from a source and test layout and 
> build perspective. My understanding here is that someone will have to 
> regularly update the wrapper jtreg tests manually using the script. This in 
> addition to having to implement this very convoluted redirection logic 
> because the tests are in the wrong place. Wouldn't it make a lot more sense 
> to put the complicated logic in the import procedure for graal source so that 
> it would fit nicely into the OpenJDK source/build/test model, instead of 
> adding more and more complicated workarounds in the OpenJDK build and test 
> procedures?)

Yes, the updategraalinopenjdk command[1] can be modified in anyway seen fit to 
make Graal a better citizen when copied to JDK. It would also be ok to add the 
jtreg commands in test source headers.

-Doug

[1] 
https://github.com/oracle/graal/blob/27288e546392f44ebf8107795647e0db155faf38/compiler/mx.compiler/mx_compiler.py#L1161

> 
> On 2018-06-18 22:26, Ekaterina Pavlova wrote:
>> Hi All,
>> 
>> please review porting of Graal unit tests under jtreg. The main idea of this 
>> porting is to keep Graal unit tests as is
>> (located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them 
>> similar way they are run in Graal project.
>> To achieve this 
>> test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java 
>> helper class has been written
>> which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run 
>> specified set of Graal unit tests. The groups of
>> Graal unit tests to run are defined in auto-generated 
>> test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests.
>> 
>> New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into 
>> jdk.vm.compiler.tests.jar and then install
>> it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/.
>> 
> GRAALUNIT_LIB is never defined (in the open). Since this is used in open 
> makefiles, we need an open configure option to set it.
> 
> To make things clearer, I would rename LIB_DIR to something like 
> LIB_OUTPUTDIR to signal that this is a location to put files in, rather than 
> read them from.
> 
> FLATTEN has no effect in the SetupCopyFiles call since all the jar files 
> found by wildcard can only be in one directory anyway.
> 
> BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests classes 
> and jars should be built into $(SUPPORT_OUTPUTDIR)/test/... Please create a 
> suitable sub directory there for the output from this makefile.
> 
> The all and test-image targets are never called so no need to declare them.
> 
> There are some style issues too. (Please see 
> http://openjdk.java.net/groups/build/doc/code-conventions.html for details.)
> * 43 logic indent 2 spaces
> * 52 we like to put the ending )) on a new line with ', \' at the end of the 
> line before
> * 58 continuation indent 4 spaces
> * 88, 89 please break long lines
> * 90 continuation indent 4 spaces
> * 99 continuation indent 4 spaces
> * 120 break before ))
>> make/Main.gmk adds proper dependencies for build-test-hotspot-jtreg-graal 
>> and test-image-hotspot-jtreg-graal.
>> 
> The target build-test-hotspot-jtreg-graal only needs to depend on 
> exploded-image-optimize.
> 
> The new graal specific targets should be guarded by INCLUDE_GRAAL in 
> Main.gmk. Otherwise those targets will silently do nothing if invoked without 
> graal. That means adding them to JVM_TEST_IMAGE_TARGETS needs to be 
> conditional too.
>> test/TestCommon.gmk passes TEST_IMAGE_GRAAL_DIR to jtreg so jtreg knows 
>> where to find Graal tests and libs.
>> 
> This needs to be duplicated for make/RunTest.gmk so that these tests work 
> with "make run-test" as well.
> 
> /Erik
>> test/hotspot/jtreg/compiler/graalunit/TestPackages.txt file defines 
>> "testName -> testPrefix [requiresStatement]" mapping
>> which is used by test/hotspot/jtreg/compiler/graalunit/generateTests.sh 
>> script to generate jtreg tests.
>> 
>> test/hotspot/jtreg/compiler/graalunit/com.oracle.mxtool.junit was ported 
>> from mx project without any changes.
>> 
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8205207
>>  webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.00/index.html
>> testing: new tests were executed as part of automatic HS testing for several 
>> months.
>> 
>> 
>> thanks,
>> -katya
>> 
>> p.s.
>>  Igor Ignatyev volunteered to sponsor this change. 
> 

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems

[Doug Simon]

> On 19 May 2017, at 11:15, Magnus Ihse Bursie  
> wrote:
> 
> 
> On 2017-05-19 09:15, David Holmes wrote:
>> Hi Magnus,
>> 
>> On 18/05/2017 8:06 PM, Magnus Ihse Bursie wrote:
>>> 
>>> 
>>> On 2017-05-18 09:35, David Holmes wrote:
 On 18/05/2017 5:32 PM, Magnus Ihse Bursie wrote:
> On 2017-05-18 08:25, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8174231
>> 
>> webrevs:
>> 
>> Build-related: http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/
> 
> Build changes look  good.
 
 Thanks Magnus! I just realized I left in the AC_MSG_NOTICE debugging 
 prints outs - do you want me to remove them? I suppose they may be useful 
 if something goes wrong on some platform.
>>> 
>>> I didn't even notice them. :-/
>>> 
>>> It's a bit unfortunate we don't have a debug level on the logging from 
>>> configure. :-( Otherwise they would have clearly belonged there.
>>> 
>>> The AC_MSG_NOTICE messages stands out much from the rest of the configure 
>>> log, so maybe it's better that you remove them. The logic itself is very 
>>> simple, if the -D flags are missing then we can surely tell what happened. 
>>> So yes, please remove them.
>> 
>> Webrev updated in place.
> Code looks good!
> 
> In the future, I very much prefer if you do not update webrevs in place. It's 
> hopeless if you start reading a thread after some updates have occured, the 
> mails don't make any sense, and it's hard to follow after-the-fact how the 
> patch evolved.

Is there any chance openjdk code reviewing will adopt a slightly more modern 
process than webrevs such as Crucible where a full history of code evolution 
during a review is preserved?

-Doug

Re: [9] RFR[L] 8174879: Rename jdk.vm.ci to jdk.internal.vm.ci

[Doug Simon]

> On 16 Feb 2017, at 19:30, Vladimir Kozlov <vladimir.koz...@oracle.com> wrote:
> 
> Hi Doug,
> 
> Is it because of next change?:
> 
>   module jdk.internal.vm.ci {
> -  exports jdk.vm.ci.services;
> +  exports jdk.vm.ci.services to jdk.internal.vm.compiler;
> 
> But you said before that your version of graal has the same module name. Why 
> you need --add-exports?

I am projecting ahead to the bug fix Mandy talks about in 
http://mail.openjdk.java.net/pipermail/graal-dev/2017-February/004889.html 
which means patching in an external version of Graal will no longer benefit 
from the qualified exports of JVMCI to jdk.vm.compiler.

-Doug

> On 2/16/17 2:01 AM, Doug Simon wrote:
>> Just to note here, this means an external version of Graal will now have to 
>> use --add-exports VM options to access JVMCI. Which is ok since additional 
>> VM options are required anyway to put an external Graal on the module path.
>> 
>> -Doug
>> 
>>> On 16 Feb 2017, at 08:54, Magnus Ihse Bursie 
>>> <magnus.ihse.bur...@oracle.com> wrote:
>>> 
>>> On 2017-02-16 02:37, Vladimir Kozlov wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8174879
>>>> 
>>>> jdk.vm.ci and jdk.vm.compiler are purely JVM internal modules that is only 
>>>> of interest to VM developers (and researchers), not general Java 
>>>> developers. It'd be appropriate for it to be an internal module and not to 
>>>> export any API.
>>>> 
>>>> Rename jdk.vm.ci and jdk.vm.compiler modules to jdk.internal.vm.ci and 
>>>> jdk.internal.vm.compiler. No packages renaming.
>>>> Exports jdk.vm.ci.services only to jdk.internal.vm.compiler.
>>>> 
>>>> Webrevs:
>>>> 
>>>> top: http://cr.openjdk.java.net/~kvn/8174879/webrev.top/
>>>> jdk: http://cr.openjdk.java.net/~kvn/8174879/webrev.jdk/
>>>> hotspot: http://cr.openjdk.java.net/~kvn/8174879/webrev.hs/
>>> 
>>> Looks good to me.
>>> 
>>> /Magnus
>>>> 
>>>> Tested with RBT.
>>>> 
>>>> Thanks,
>>>> Vladimir
>>> 
>> 

Re: [9] RFR[L] 8174879: Rename jdk.vm.ci to jdk.internal.vm.ci

[Doug Simon]

Just to note here, this means an external version of Graal will now have to use 
--add-exports VM options to access JVMCI. Which is ok since additional VM 
options are required anyway to put an external Graal on the module path.

-Doug

> On 16 Feb 2017, at 08:54, Magnus Ihse Bursie  
> wrote:
> 
> On 2017-02-16 02:37, Vladimir Kozlov wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8174879 
>> 
>> jdk.vm.ci and jdk.vm.compiler are purely JVM internal modules that is only 
>> of interest to VM developers (and researchers), not general Java developers. 
>> It'd be appropriate for it to be an internal module and not to export any 
>> API. 
>> 
>> Rename jdk.vm.ci and jdk.vm.compiler modules to jdk.internal.vm.ci and 
>> jdk.internal.vm.compiler. No packages renaming. 
>> Exports jdk.vm.ci.services only to jdk.internal.vm.compiler. 
>> 
>> Webrevs: 
>> 
>> top: http://cr.openjdk.java.net/~kvn/8174879/webrev.top/ 
>> jdk: http://cr.openjdk.java.net/~kvn/8174879/webrev.jdk/ 
>> hotspot: http://cr.openjdk.java.net/~kvn/8174879/webrev.hs/ 
> 
> Looks good to me.
> 
> /Magnus
>> 
>> Tested with RBT. 
>> 
>> Thanks, 
>> Vladimir 
> 

Re: RFR: JDK-8172548 unpack200 fails linking with new update of SS12u4

[Doug Simon]

> On 2 Feb 2017, at 13:07, Magnus Ihse Bursie  
> wrote:
> 
> When building with Solaris Studio 12u4, linking of unpack200 fails. The fix 
> is to include "environ" in the mapfile.
> 
> This patch is contributed by Douglas Simon.

For full disclosure, the original patch and discovery was by Stefan Anzinger.

> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8172548
> Patch inline:
> diff --git a/make/mapfiles/libunpack/mapfile-vers-unpack200-solaris-sparc 
> b/make/mapfiles/libunpack/mapfile-vers-unpack200-solaris-sparc
> --- a/make/mapfiles/libunpack/mapfile-vers-unpack200-solaris-sparc
> +++ b/make/mapfiles/libunpack/mapfile-vers-unpack200-solaris-sparc
> @@ -28,6 +28,7 @@
>  SUNWprivate_1.1 {
> global:
> # These are needed by the c runtime in SS12u4
> +   environ;
> _environ;
> __environ_lock;
> ___Argv;
> 
> 
> /Magnus

Re: [9] RFR(XL) 8166417: Integrate Graal-core into JDK for AOT compiler

[Doug Simon]

> On 9 Dec 2016, at 08:07, Vladimir Kozlov  wrote:
> 
> Thank you, Mandy
> 
> On 12/8/16 7:46 PM, Mandy Chung wrote:
>> 
>>> On Dec 7, 2016, at 2:10 PM, Vladimir Kozlov  
>>> wrote:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8166417
>>> 
>>> It is part of JEP 295: Ahead-of-Time Compilation
>>> https://bugs.openjdk.java.net/browse/JDK-8166089
>>> 
>>> http://cr.openjdk.java.net/~kvn/8166417/top.webrev/
>>> http://cr.openjdk.java.net/~kvn/8166417/jdk.webrev/
>> 
>> I reviewed module-info.java.*.
>> 
>> src/java.base/share/classes/module-info.java.extra
>> 
>> Now I realized jdk.vm.compiler may be excluded from JDK time at
>> build/configure time.  That explains why you can’t add these
>> qualified exports to module-info.java.
>> 
>> As we discussed offline, you can move this file to unix/classes
>> and if jdk.vm.compiler is not included in the JDK build, the build
>> tool that generates gensrc/java.base/module-info.java would filter
>> out these qualified exports.
> 
> Done. And it works.
> 
>> 
>> Related to the use of jdk.internal.misc.VM, you mentioned separately
>> that Graal uses jdk.internal.misc.VM.savedProps to find properties with
>> GRAAL_OPTION_PROPERTY_PREFIX.  Why can’t you use System.getProperties?
>> savedProps stores the properties that are removed from system properties.
>> Are Graal’s properties not in system properties?
> 
> I asked Graal guys. May be Doug can answer.


The reason we use VM.savedProps is that initialization of JVMCI and Graal is 
lazy (to avoid impacting VM startup) and can take place after application code 
has been executed. As such, we want to avoid the possibility of tampering with 
the system properties that configure JVMCI and Graal (i.e., all “jvmci.*” and 
“graal.*” properties).

-Doug

Re: [9] RFR(XL) 8166417: Integrate Graal-core into JDK for AOT compiler

[Doug Simon]

> On 8 Dec 2016, at 13:50, Magnus Ihse Bursie  
> wrote:
> 
> On 2016-12-07 23:10, Vladimir Kozlov wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8166417
>> 
>> It is part of JEP 295: Ahead-of-Time Compilation
>> https://bugs.openjdk.java.net/browse/JDK-8166089
>> 
>> http://cr.openjdk.java.net/~kvn/8166417/top.webrev/
>> http://cr.openjdk.java.net/~kvn/8166417/jdk.webrev/
>> http://cr.openjdk.java.net/~kvn/8166417/hotspot.webrev/
> 
> * In CompileJavaModules.gmk, there is an *extreme* list of excludes. I note 
> that most of them end in ".test". The proper solution to this is to move the 
> test code to the test/ directory instead of the src/ directory. Also, four of 
> them are the javac processor build tools. These should move to make/src, as 
> other build tools.

Keeping like it is simplifies pulling changes from 
https://github.com/graalvm/graal-core. It's possible to write a more 
sophisticated tool for pulling changes in from the github and places the 
sources into more openjdk compatible locations. It's up to the HotSpot compiler 
team whether investing in such a tool makes sense.

> * In hotspot.m4: AC_MSG_CHECKING must always be matched by a AC_MSG_RESULT, 
> even before AC_MSG_ERROR, otherwise the output will be garbled.
> 
> * In hotspot.m4: The test to determine if we should set INCLUDE_GRAAL is 
> incorrect. It only tests for the default value, not for the explicitely set 
> (i.e., HOTSPOT_CHECK_JVM_FEATURE(graal)).
> 
> * The file hotspot/make/Tools.gmk is broken ("ifeq ($(ENABLE_AOT), true)" 
> will never evaluate to true). But on the other hand, it is not needed, and 
> should just be removed. :-)
> 
> * The file hotspot/src/jdk.vm.compiler/share/classes/overview.html seems a 
> bit dangling. Is this supposed to be included in some Javadoc generation? The 
> html file refers to a projects.jpg and projects.html that does not exist. 
> Maybe it should just be removed?

I would recommend omitting all graal-core overview.html files in OpenJDK.

> * Finally, just for the record, I too think the source should be structured 
> according to the OpenJDK praxis. (But I won't fight about this.)

-Doug

>> 
>> This is formal review request for integration Graal-core sources into 
>> OpenJDK. AOT compiler uses Graal-core as backend compiler. We need to 
>> integrated Graal-core sources into JDK and add build changes to build Graal 
>> module.
>> 
>> Note, changes are based on latest jdk9/hs sources which do not have latest 
>> jigsaw update yet. With jigsaw update small changes will be done to 
>> module-info.java.extra in java.base:
>> 
>>  exports jdk.internal.misc to jdk.vm.compiler;
>> + opens jdk.internal.misc to jdk.vm.compiler;
>> 
>> - exports com.sun.crypto.provider to jdk.vm.compiler;
>> + opens com.sun.crypto.provider to jdk.vm.compiler;
>> 
>> And changes in top make/GensrcModuleInfo.gmk will not be needed.
>> 
>> 
>> 
>> Graal is a dynamic compiler written in Java that integrates with the HotSpot 
>> JVM. It has a focus on high performance and extensibility. In addition, it 
>> provides optimized performance for Truffle based languages running on the 
>> JVM.
>> 
>> https://github.com/graalvm/graal-core
>> 
>> Oracle Labs is developing and maintaining it.
>> 
>> Here are people who contributed into Graal development (sorry if someone is 
>> missing or misspelled, please speak):
>> 
>> ~70k LOC: Douglas Simon
>> ~60k LOC: Lukas Stadler
>> ~30k LOC: Thomas Wuerthinger
>> ~30k LOC: Tom Rodriguez
>> ~30k LOC: Roland Schatz
>> ~30k LOC: Josef Eisl
>> ~30k LOC: Christian Wimmer
>> ~16k LOC: Chris Thalinger
>> ~13k LOC: Gilles Duboscq
>> ~11k LOC: David Leopoldseder
>> ~ 8k LOC: Stefan Anzinger
>> ~ 8k LOC: Christian Humer
>> 
>> Other contributors >100 LOC in approximate order of contribution size:
>> Michael Berg, Bernhard Urban, Miguel Garcia, Yudi Zheng, Christos 
>> Kotselidis, Andreas Woess, Stefan Rumzucker, Aleksandar Prokopec, Christian 
>> Haeubl, Morris Meyer, Matthias Grimmer, Erik Eckstein, Josef Haider, Manuel 
>> Rigger, Michael Haupt, Niclas Adlertz, Jaroslav Tulach, Chris Seaton, Peter 
>> B. Kessler, Christian Wirth, Benoit Daloze.
>> 
>> 
>> Thanks,
>> Vladimir
> 

Re: [9] RFR(M) 8166416: [AOT] Integrate JDK build changes and launcher 'jaotc' for AOT compiler

[Doug Simon]

> On 27 Oct 2016, at 20:12, Vladimir Kozlov  wrote:
> 
> On 10/27/16 10:55 AM, Christian Thalinger wrote:
>> 
>>> On Oct 27, 2016, at 2:40 AM, Erik Joelsson  wrote:
>>> 
>>> 
>>> 
>>> On 2016-10-27 02:45, Vladimir Kozlov wrote:
 AOT JEP:
 https://bugs.openjdk.java.net/browse/JDK-8166089
 Subtask:
 https://bugs.openjdk.java.net/browse/JDK-8166416
 Webrev:
 http://cr.openjdk.java.net/~kvn/aot/top.webrev/
>>> hotspot.m4: 296: Comment is misleading. Should just be removed.
>>> 
>>> CompileJavaModules.gmk: Use of -g flag for java compilation is controlled 
>>> globally. Please remove.
>> 
>> There is a reason for that.  Some debugging related Graal code makes use of 
>> classfile information to provide better information.  Since this is Java and 
>> not C++ it *is* possible to have pleasant debugging experience even in 
>> product builds.  I want this to be there.
> 
> Chris, do we need -g for JVMCI module too for that?

I’m assuming Chris is referring to the use of debug info for snippet parameter 
names:

https://github.com/graalvm/graal-core/blob/7c94891d06f08a635367df6078696b9388332f3b/graal/com.oracle.graal.replacements/src/com/oracle/graal/replacements/SnippetTemplate.java#L191

For that use case, -g is not necessary for JVMCI. That said, I think it’s 
always useful for Java code to be compiled with -g ;-)

-Doug

>> 
>>> 
>>> Main.gmk: buildtools-hotspot should be declared inside the 
>>> CREATING_BUILDJDK conditional like all other buildtools targets.
>>> 
 http://cr.openjdk.java.net/~kvn/aot/jdk.webrev/
>>> The extra exports from java.base needs to go in a new 
>>> jdk/src/java.base/share/classes/module-info.java.extra since the module 
>>> jdk.vm.compiler is optional.
 http://cr.openjdk.java.net/~kvn/aot/hs.make.webrev/
>>> Lib-jdk.aot.gmk: Please inline LDFLAGS and LIBS and add $(LIBS_JDKLIB) to 
>>> LIBS since that will provide -lc on Solaris automatically. No need to set 
>>> DEBUG_SYMBOLS or STRIP_SYMBOLS as the defaults should be correct and 
>>> controlled globally.
>>> 
>>> /Erik
 
 Please, review build changes for AOT.  Only Linux/x64 platform is 
 supported. 'jaotc' and AOT part of Hotspot will be build only on Linux/x64.
 
 Changes include new 'jaotc' launcher, makefile changes to build 
 jdk.vm.compiler (Graal) and jdk.aot modules used by 'jaotc'.
 Both modules sources are located in Hotspot: hotspot/src/jdk.aot and 
 hotspot/src/jdk.vm.compiler.
 'jaotc' requires installed libelf package on a system to build native part 
 of 'jaotc'. It is used to generated AOT shared libraries (.so) as result 
 of AOT compilation.
 
 Hotspot makefile changes will be pushed together with Hotspot AOT changes.
 
 Thanks,
 Vladimir

Re: [9] RFR (xs) 8168317: [JVMCI] use reflection instead of jdk 9 Module API in Services.java

[Doug Simon]

> On 24 Oct 2016, at 23:24, Christian Thalinger  wrote:
> 
> 
>> On Oct 24, 2016, at 3:48 AM, Erik Joelsson  wrote:
>> 
>> Adding build-dev, which should be included when discussing build issues. For 
>> any new readers, please see [1] for the full discussion.
>> In theory it is possible to compile against and run on the exploded image 
>> during the build, but I do not recommend it. Igor is correct in the build 
>> team being against that design. The rationale is that it adds a lot of 
>> complexity to the build. The exploded image cannot be safely run until all 
>> java modules have been compiled as it introduces races. Maintaining the 
>> build with such a construct will be very brittle when other changes are 
>> made. We did allow the gensrc for jdk.vm.ci to run in this way for a short 
>> time, since it was only supported on Linux x64, where these races are rarer, 
>> but if this would ever need to be built on Windows, we would be in trouble 
>> quickly. Luckily, jdk.vm.ci was able to refactor away from needing this 
>> annotation processing for that module. 
>> I certainly prefer the reflection solution proposed here, but find it sad 
>> that it's needed. 
>> 
> 
> The problem I have with this solution is that jdk.vm.ci code is now 
> restricted to JDK N-1 code.  This might be ok right now because we are able 
> to work around that issue with reflection but when important features like 
> Valhalla come around this is a problem.
> 
> Value types will be hugely important for JVMCI and its compilers and we 
> simply cannot just skip one JDK release just because the build system can’t 
> handle it.

I agree. However, I suspect large parts of the JDK will also want to leverage 
Valhalla so I’m guessing the build problem will have to be solved anyway at 
some point. Maybe it worth filing a tracking bug to revert these changes at 
that time.

-Doug

> 
>> 
>> /Erik
>> 
>> [1] 
>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2016-October/024743.html
>> 
>> On 2016-10-19 21:54, Igor Veresov wrote:
>>> 
 On Oct 19, 2016, at 12:47 PM, Christian Thalinger  
 wrote:
 
 
> On Oct 19, 2016, at 9:40 AM, Vladimir Kozlov  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8168317
> 
> webrev:
> http://cr.openjdk.java.net/~kvn/8168317/webrev/
> 
> When Graal is built as part of JDK it requires first to build an 
> annotation processor using boot jdk 8.
> After JDK-8167180 changes Services class is referenced by annotation 
> processor but the code is using jdk 9 Module API and it can't be used 
> with jdk 8.
 
 I left a comment in the bug: Permalink
 
 Basically, it should be possible to use the newly built javac to compile 
 the annotation processors.  Erik?
>>> 
>>> It’s not only about compilation it’s about running it on the bootstrap JDK, 
>>> which in currently 8.
>>> 
>>> igor
>>> 
 
 Can you paste or upload the .gmk file?
 
> 
> Use reflection instead of Module API and use code only for running with 
> jdk 9.
> 
> Testing with JPRT and JDK build of Graal.
> 
> Thanks,
> Vladimir
 
>>> 
>> 
> 

Re: Setting/overriding HOTSPOT_VERSION_STRING

[Doug Simon]

Thanks Volker!

I think --with-version-opt is what I need.

-Doug

> On 05 Jul 2016, at 14:34, Volker Simonis <volker.simo...@gmail.com> wrote:
> 
> Hi Doug,
> 
> HOTSPOT_VERSION_STRING comes from VERSION_STRING:
> 
> hotspot/make/lib/CompileJvm.gmk:
> -DHOTSPOT_VERSION_STRING='"$(VERSION_STRING)"
> 
> and VERSION_STRING comes from configure. In your build directory you can do:
> 
> grep VERSION_STRING spec.gmk
> VERSION_STRING := 9-internal+0-2016-06-29-155201.d046063.jdk9-hs-comp
>-DVERSION_STRING='"$(VERSION_STRING)"' \
> 
> You can influence this version number with various configure options:
> 
> configure --help | grep "with-version"
>  --with-version-string   Set version string [calculated]
>  --with-version-pre  Set the base part of the version 'PRE' field
>  --with-version-opt  Set version 'OPT' field (build metadata)
>  --with-version-buildSet version 'BUILD' field (build number) [not
>  --with-version-majorSet version 'MAJOR' field (first number) [current
>  --with-version-minorSet version 'MINOR' field (second number) [current
>  --with-version-security Set version 'SECURITY' field (third number) [current
>  --with-version-patchSet version 'PATCH' field (fourth number) [not
> 
> Regards,
> Volker
> 
> PS: the correct list for build related questions is build-dev.
> build-infra-dev was for the development of the new build system.
> 
> 
> 
> 
> 
> 
> On Tue, Jul 5, 2016 at 12:21 PM, Doug Simon <doug.si...@oracle.com> wrote:
>> Hi,
>> 
>> I’m trying to customize the value set during the JDK 9 build for the 
>> java.vm.version system property. As far as I can see, it comes from 
>> HOTSPOT_VERSION_STRING.
>> 
>> vm_version.cpp:
>> 
>> #define VM_RELEASE HOTSPOT_VERSION_STRING
>> 
>> const char* Abstract_VM_Version::vm_release() {
>> return VM_RELEASE;
>> }
>> 
>> arguments.cpp:
>> 
>> PropertyList_add(&_system_properties, new SystemProperty("java.vm.version", 
>> VM_Version::vm_release(),  false));
>> 
>> Do you know how I can set/override the value of HOTSPOT_VERSION_STRING 
>> passed to the HotSpot build? I tried setting it as an environment variable 
>> but it didn’t work:
>> 
>> dsimon@kurz ~/hs-comp> touch hotspot/src/share/vm/runtime/vm_version.cpp; 
>> env HOTSPOT_VERSION_STRING=X make CONF=release
>> Building configuration 'macosx-x86_64-normal-server-release' (matching 
>> CONF=release)
>> Building target 'default (exploded-image)' in configuration 
>> 'macosx-x86_64-normal-server-release'
>> Building JVM variant 'server' with features 'all-gcs cds closed-src 
>> commercial-features compiler1 compiler2 dtrace fprof jni-check jvmci jvmti 
>> management nmt services trace vm-structs'
>> Updating libjvm.dylib due to 1 file(s)
>> Finished building target 'default (exploded-image)' in configuration 
>> 'macosx-x86_64-normal-server-release'
>> dsimon@kurz ~/hs-comp> 
>> build/macosx-x86_64-normal-server-release/jdk/bin/java -version
>> java version "9-internal"
>> Java(TM) SE Runtime Environment (build 
>> 9-internal+0-2016-06-29-123036.dsimon.hs-comp)
>> Java HotSpot(TM) 64-Bit Server VM (build 
>> 9-internal+0-2016-06-29-123036.dsimon.hs-comp, mixed mode)
>> 
>> -Doug
>> 

Re: code review round 0 for minor FDS makefile fix (8033714)

[Doug Simon]

Not sure if I’m being asked for a review, but if so, looks good.

On Feb 6, 2014, at 5:07 PM, Tom Rodriguez tom.rodrig...@oracle.com wrote:

 Looks good to me too.  Thanks for fixing this.
 
 tom
 
 On Feb 6, 2014, at 6:07 AM, Daniel D. Daugherty daniel.daughe...@oracle.com 
 wrote:
 
 On 2/5/14 9:28 PM, David Holmes wrote:
 Hi Dan,
 
 Looks good to me.
 
 Thanks for the review!
 
 
 (I never run the install targets :( )
 
 Neither do I and apparently neither does JPRT... That's how this
 slipped through the cracks...
 
 Dan
 
 
 
 Thanks,
 David
 
 On 6/02/2014 9:20 AM, Daniel D. Daugherty wrote:
 This code review request is going to three different aliases.
 Don't use Thunderbird's reply to list option since it will
 pick just _one_ of the _three_ lists.
 
 
 Greetings,
 
 Doug Simon and Tom Rodriguez have sent a Full Debug Symbols (FDS)
 makefile fix our way. Here are the bug and webrev URLs:
 
 http://cr.openjdk.java.net/~dcubed/8033714-webrev/0-jdk9-hs-runtime/
 
8033714 hotspot 'install_jvm' bld target broken with
 ZIP_DEBUGINFO_FILES=0
https://bugs.openjdk.java.net/browse/JDK-8033714
 
 As you might guess from the bug synopsis, this fix is needed when
 building without ZIP'ing the debuginfo files (ZIP_DEBUGINFO_FILES=0).
 Based on the Graal project fix, I've updated a few other places where
 building with FDS disabled is affected.
 
 As always, comments and suggestions are welcome.
 
 Dan