On Tue, 12 Jan 2021 05:04:11 GMT, Igor Veresov <ivere...@openjdk.org> wrote:
>> This change removes the legacy compilation policy and an emulation mode to
>> the tiered policy to simulate the old behavior with
>> ```-XX:-TieredCompilation```. The change removed a bunch of interpreter
>> code, devirtualizes the compilation policy API, adds a consistent way to
>> query compiler configuration with the new ```CompilerConfig``` API.
>>
>> I've tested this with hs-tier{1,2,3,4,5}. And also made sure it builds and
>> works with C1/C2-Graal/AOT being enabled/disabled.
>>
>> Since there are platform-specific changes I would greatly appreciate some
>> help from the maintainers of the specific ports to verify the build and run
>> basic smoke tests. I've already tested x64 and aarch64. Thanks!
>
> Igor Veresov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Check legacy flags validity before deriving flag values for emulation mode.
I would also suggest to do performance runs. Ask Eric for help. Changes are
significant and may affect performance due to some typo.
src/hotspot/share/compiler/compilerDefinitions.hpp line 234:
> 232: static bool is_interpreter_only() {
> 233: return Arguments::is_interpreter_only() || TieredStopAtLevel ==
> CompLevel_none;
> 234: }
Can you move these functions after has_*() functions? They are used before.
src/hotspot/share/compiler/compilerDefinitions.hpp line 179:
> 177: }
> 178: // Is the JVM in a configuration that permits only c2-compiled methods?
> 179: // JVMCI compiler replaces C2.
The comment `JVMCI compiler replaces C2.` should be removed or moved to
`is_jvmci_compiler_only()` where it is make more sense.
src/hotspot/share/compiler/compilerDefinitions.hpp line 171:
> 169: }
> 170:
> 171: static bool is_c2_available() {
For me `_available` suffix sounds similar to `has_`. May be `_enabled` is
better. At least it is less confusing where it is called.
src/hotspot/cpu/aarch64/c1_LIRGenerator_aarch64.cpp line 1414:
> 1412: // use LD;DMB but stores use STLR. This can happen if C2 compiles
> 1413: // the stores in one method and C1 compiles the loads in another.
> 1414: if (!CompilerConfig::is_c1_or_interpreter_only_no_aot_or_jvmci()) {
It is C1 code which should not be executed in -Xint. Why check
`interpreter_only`?
src/hotspot/cpu/aarch64/gc/shenandoah/c1/shenandoahBarrierSetC1_aarch64.cpp
line 54:
> 52: ShenandoahBarrierSet::assembler()->cmpxchg_oop(masm->masm(), addr,
> cmpval, newval, /*acquire*/ true, /*release*/ true, /*is_cae*/ false, result);
> 53:
> 54: if (CompilerConfig::is_c1_or_interpreter_only_no_aot_or_jvmci()) {
Again about `interpreter_only` check.
src/hotspot/share/compiler/compilerDefinitions.hpp line 243:
> 241: static bool is_c1_only_no_aot_or_jvmci() {
> 242: return is_c1_only() && !is_aot() && !is_jvmci();
> 243: }
These names are a little confusing: what about C2, why only no AOT and JVMCI. I
understand that you want to check if JVMCI or AOT can install their compiled
code.
May be `is_c1_nmethods_only`, `is_c1_nmethods_or_interpreter_only` ?
src/hotspot/share/oops/method.cpp line 1013:
> 1011: return false;
> 1012: if (comp_level == CompLevel_any)
> 1013: return is_not_c1_compilable() && is_not_c2_compilable();
Was it bug before?
src/hotspot/share/compiler/compilerDefinitions.cpp line 62:
> 60: }
> 61: } else if (strcmp(CompilationMode, "high-only") == 0) {
> 62: if (!CompilerConfig::has_c2() &&
> !CompilerConfig::is_jvmci_compiler()) {
Is using `!is_c2_or_jvmci_compiler_available()` better here? All flags should
be processed at this point. And we could have some `TieredStopAtLevel` flags.
It looks like you have cross dependency between CompilerConfig and
CompilationModeFlag which make using `is_c2_or_jvmci_compiler_available()`
impossible here.
src/hotspot/share/compiler/compilerDefinitions.cpp line 84:
> 82: } else if (CompilerConfig::is_c2_or_jvmci_compiler_only()) {
> 83: _mode = Mode::HIGH_ONLY;
> 84: } else if (CompilerConfig::is_jvmci_compiler() && !TieredCompilation)
> {
Should you check `CompilerConfig::is_tiered()` instead of `TieredCompilation`
flag?
-------------
PR: https://git.openjdk.java.net/jdk/pull/1985
