On Fri, 19 Apr 2024 15:50:05 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> Scott Gibbons has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add enter() and leave(); remove Windows-specific register stuff
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2603:
>
>> 2601: const Register wide_value = rax;
>> 2602: const Register rScratch1 = r10;
>> 2603:
>
> Maybe put an `assert_different_registers` here for the above registers, just
> to be sure. (I see you are avoiding the existing `rscratch1` already, because
> of a conflict with `c_rarg2`)
Done.
> src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2674:
>
>> 2672: // Parameter order is (ptr, byteVal, size)
>> 2673: __ xchgq(c_rarg1, c_rarg2);
>> 2674: __ pop(rbp); // Clear effect of enter()
>
> Why not just use `leave()` here?
No special reason. I've changed it since it seems to provide more clarity.
> src/hotspot/share/opto/library_call.cpp line 4959:
>
>> 4957: if (stopped()) return true;
>> 4958:
>> 4959: if (StubRoutines::unsafe_setmemory() == nullptr) return false;
>
> I don't see why this check is needed here, since we already check whether the
> stub is there in `is_intrinsic_supported`.
>
> Note that `inline_unsafe_copyMemory` also doesn't have this check. I think it
> would be good to keep consistency between the two.
Removed.
> src/hotspot/share/opto/runtime.cpp line 780:
>
>> 778: const TypeFunc* OptoRuntime::make_setmemory_Type() {
>> 779: // create input type (domain)
>> 780: int num_args = 4;
>
> This variable seems redundant.
It is. It is there due to copy/paste from the other 10 places that also have
the same redundant variable declaration. I've removed it from here, but I
think I'll be asked to submit a separate PR if I remove it from the other
locations.
Note that it's also redundant in `make_arraycopy_Type()`.
> src/hotspot/share/opto/runtime.cpp line 786:
>
>> 784: fields[argp++] = TypePtr::NOTNULL; // dest
>> 785: fields[argp++] = TypeLong::LONG; // size
>> 786: fields[argp++] = Type::HALF; // size
>
> Since the size is a `size_t`, I don't think this is correct on 32-bit
> platforms. I think here we want `TypeX_X`, and then add the extra `HALF` only
> on 64-bit platforms. Similar to what we do in `make_arraycopy_Type`:
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/runtime.cpp#L799-L802
>
> (Note that you will also have to adjust `argcnt` for this)
I don't understand this well enough to be confident in the change. Can you
please verify that I've changed it properly?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572797332
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572800059
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572804660
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572815040
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572823468
