RFR: 8343767: Enumerate StubGen blobs, stubs and entries and generate code from declarations

[Andrew Dinn]

On Thu, 7 Nov 2024 18:54:10 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> Implementation of JDK-8343767
>
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 208:
> 
>> 206:     StubGenStubId stub_id = StubGenStubId::call_stub_id;
>> 207:     const char* stub_name = StubRoutines::get_stub_name(stub_id);
>> 208:     StubCodeMark mark(this, "StubRoutines", stub_name);
> 
> Cleanup suggestion: `StubGenStubId` could be passed as an argument into `StubCodeMark` constructor and name lookup performed there instead. The pattern is very common.

That's a nice idea. I was thinking that when we come to save and restore the code in the AOT cache we would need the name again. However, looking at the prototype I think we can also pass the id to the load and save routines and let them lookup the name when they need it.

> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 427:
> 
>> 425:   address generate_catch_exception() {
>> 426:     StubGenStubId stub_id = StubGenStubId::catch_exception_id;
>> 427:     StubCodeMark mark(this, stub_id);
> 
> Stylistic suggestion: for such trivial cases, you can simply pass `StubGenStubId` constant as an argument. No need to declare a local for it.

When we come to add save and restore we will need to have the id to do the AOT cache lookup then generate the stub if not found on a production run as well as to write the stub if this is an assembly run. So, I'll leave this a is for then.

Also, at some point I want to add an assert that the stub generator should be 'expecting' this stub which means I will need to pass it this id. A stub generator knows which blob it is writing into (it gets passed the blob id when constructed) and the template macros generate a debug-only method to translate a stub id to the blob (id) named in the stub declaration. So, we can use that to cross-check that the declared blob matches the one the stub gets written to. Quite a few (generic) stubs were being written to different blobs by different arches so it will be good to ensure they end up in the same blob.

That's not 100% the full picture just now because we also have StubGenerator instances responsible for writing the icache flush stub and (on x86 only) vm_version stubs and they have no associated blob id. I'd like to fold those stubs and their associated blobs into the declaration structure too at some point. For now I'll make sure they are associated with blob_id == NO_BLOBID.

> src/hotspot/share/runtime/stubDeclarations.hpp line 738:
> 
>> 736:   do_entry(compiler, bigIntegerLeftShiftWorker,                         \
>> 737:            bigIntegerLeftShiftWorker, bigIntegerLeftShift)              \
>> 738:   AARCH64_ONLY(                                                         \
> 
> It's unfortunate to see platform-specific declarations in `stubDeclarations.hpp`. Any chances to keep them in CPU-specific files (as it is shaped now)?

That's a good question and I believe it is the preferred option.

It would certainly be possible to include a per-arch file (stubDeclarations_<arch>.hpp) to define a common (per-blob) declaration macro providing the arch-specific declarations.

e.g. for x86 the arch file would specify:

    #define STUBGEN_INITIAL_BLOBS_ARCH_DO(do_arch_blob, do_stub, repeat_stub, do_arch_entry) \
      do_arch_blob(initial, 20000 WINDOWS_ONLY(+1000))                    \
      do_stub(initial, verify_mxcsr)                                      \
      do_arch_entry(x86, initial, verify_mxcsr, verify_mxcsr_entry,       \
                    verify_mxcsr_entry)                                   \
      . . . 
    #define STUBGEN_FINAL_BLOBS_ARCH_DO(do_arch_blob, do_stub, repeat_stub, do_arch_entry) \
      . . .

The generic macro would then paste this in place of the AARCH64_ONLY(), X86_ONLY() sections e.g.

    #define STUBGEN_INITIAL_BLOBS_DO(do_blob, end_blob,                     \
                                     do_stub, repeat_stub,                  \
                                     do_entry, do_entry_init,               \
                                     do_arch_blob,                          \
                                     do_arch_entry, do_arch_entry_init)     \
      do_blob(initial)                                                      \
      do_stub(initial, call_stub)                                           \
      . . . 
      do_entry(initial, dlibm_tan_cot_huge, dlibm_tan_cot_huge,             \
               dlibm_tan_cot_huge)                                          \
      STUBGEN_INITIAL_BLOBS_ARCH_DO(do_arch_blob, do_stub,                  \
                                    repeat_stub, do_arch_entry)

I did originally plan to split the declarations like this. However, I ended up putting the arch declarations into the generic file in this draft for two reasons. The primary one was heuristic: I wanted to make it easy to see the declaration structure I was proposing before we committed to it. However, I also had a niggling concern that when it comes to updating the declarations it may be easier if they are all in the one place, right next to the doc comments that explain what the declaration format is and what gets generated from the declarations. My experience of writing a stub some years back tells me that it is important to make it easy for developers who are not fully familiar with the runtime to see how all the pieces fit together so I put all the pieces in one place.

That said, I believe your recommendation to split the declarations is probably better. By the time I have added PPC_ONLY, RISCV_ONLY etc sections whatever transparency is to be gained from keeping declarations together will be lost in the sheer volume of declarations, especially given that a lot of the arch declarations will appear multiple times in different arches.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21957#discussion_r1836534322
PR Review Comment: https://git.openjdk.org/jdk/pull/21957#discussion_r1842358211
PR Review Comment: https://git.openjdk.org/jdk/pull/21957#discussion_r1836534410