RFR: 8295661: CompileTask::compile_id() should be passed as int
Damon Fenacci
duke at openjdk.org
Fri Dec 16 09:39:19 UTC 2022
On Mon, 12 Dec 2022 12:12:09 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:
>> Changed return type of `CompileTask::compile_id()` from `int` to `uint`.
>> Also modified the type everywhere `compile_id` is used (parameters, local variables, string formatting, constant assignments) *as much as possible*.
>> Added *asserts* to check for valid value range where not possible.
>
> src/hotspot/share/c1/c1_Compilation.cpp line 616:
>
>> 614: Compilation::~Compilation() {
>> 615: // simulate crash during compilation
>> 616: assert(CICrashAt >= UINT_MAX || _env->compile_id() != CICrashAt, "just as planned");
>
> I'm wondering if we should remove the `CICrashAt >= UINT_MAX` part to catch an UINT overflow before it can happen?
>
> Suggestion:
>
> assert(_env->compile_id() != CICrashAt, "just as planned");
OK
> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 1182:
>
>> 1180: C2V_END
>> 1181:
>> 1182: C2V_VMENTRY_0(juint, allocateCompileId, (JNIEnv* env, jobject, ARGUMENT_PAIR(method), int entry_bci))
>
> This is called from Java code which does not have an unsigned int. I think we need to leave this as `jint` here.
Reverted to `jint`. I also added an assert to check for big `uint` values.
> src/hotspot/share/jvmci/jvmciEnv.cpp line 1625:
>
>> 1623: if (cb == (CodeBlob*) code) {
>> 1624: nmethod* nm = cb->as_nmethod_or_null();
>> 1625: assert(compile_id_snapshot >= 0, "negative compile id snapshot");
>
> That's something to check with the JVMCI experts but I'm wondering why `compile_id_snapshot` is a jlong whereas the compile id is a jint.
OK. Strange indeed!
> src/hotspot/share/jvmci/jvmciRuntime.cpp line 2025:
>
>> 2023: assert(compile_state->task()->compile_id() <= INT_MAX, "compile id too big");
>> 2024: JVMCIObject result_object = JVMCIENV->call_HotSpotJVMCIRuntime_compileMethod(receiver, jvmci_method, entry_bci,
>> 2025: (jlong) compile_state, (int) compile_state->task()->compile_id());
>
> Should be a cast to `jint`, right?
>
> Suggestion:
>
> (jlong) compile_state, (jint) compile_state->task()->compile_id());
I'm not sure. I just noticed that the signature of the called method has an `int` argument:
`JVMCIObject JVMCIEnv::call_HotSpotJVMCIRuntime_compileMethod (JVMCIObject runtime, JVMCIObject method, int entry_bci, jlong compile_state, int id)`
> src/hotspot/share/opto/idealGraphPrinter.cpp line 231:
>
>> 229: }
>> 230:
>> 231: void IdealGraphPrinter::print_prop(const char *name, unsigned int val) {
>
> Did you test these changes with IGV?
I've ran `java -XX:PrintIdealGraphLevel=4 -Xcomp -XX:PrintIdealGraphFile=graph.xml` with some extra prints in the new and old `print_prop` methods (to check that they were actually used).
> src/hotspot/share/runtime/sharedRuntime.cpp line 3106:
>
>> 3104:
>> 3105: const uint compile_id = CompileBroker::assign_compile_id(method, CompileBroker::standard_entry_bci);
>> 3106:
>
> But `compile_id` could still be 0, right?
Right! I've put the assert statement back.
-------------
PR: https://git.openjdk.org/jdk/pull/11630
More information about the hotspot-dev
mailing list