RFR: 8329331: Intrinsify Unsafe::setMemory [v21]

[Scott Gibbons]

On Fri, 19 Apr 2024 15:50:05 GMT, Jorn Vernee <jvernee at 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