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

[Tobias Hartmann]

On Mon, 12 Dec 2022 11:29:55 GMT, Damon Fenacci <duke 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.

Looks good, I added a few comments. Someone more familiar with JVMCI should have a look as well.

Marked as reviewed by thartmann (Reviewer).

Using unsigned int consistently feels appropriate to represent a compilation id, especially since we are already mixing unsigned and signed integers. But given the impact on JVMCI and the leakage into Java code which does not support an unsigned int, it's reasonable to use int consistently instead. The updated changes look good to me.

Thanks for making these changes. Looks good!

Thanks Tom. The intention of this change was mainly consistency, not avoiding a potential overflow.

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");

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.

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.

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());

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?

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?

-------------

PR: https://git.openjdk.org/jdk/pull/11630Marked as reviewed by thartmann (Reviewer).