RFR: 8295661: CompileTask::compile_id() should be passed as int

[Damon Fenacci]

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