RFR: 8318217: RISC-V: C2 VectorizedHashCode [v2]

[Yuri Gaevsky]

On Fri, 24 Nov 2023 18:54:37 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hey, I saw you already change the code in this patch to not use the specific registers, do you still face the JVM starting issue?
>
> I think the reason might be: with specific register, you can add effect as `USE_KILL ary, USE_KILL cnt`, but without specific register, currently you have to way to do so.
> But, in current patch, it does modify the ary and cnt in the intrinsic, so I wonder if the current (lastest) patch is safe enough in all situation.
> 
> It maybe be helpful to add 2 new register when matching the instrinsic in ad file, and I guess the register allocator will merge different use of temp register together?
> But I still think it's not necessary to specify the register when matching arrays_hashcode in ad file.

Hi @Hamlin-Li,

My apologies for some delay with an answer.

Without specifying the "concrete" registers for _ary/cnt/result_, as e.g. as follows:
(1) [ iRegP/iRegI ]

instruct arrays_hashcode(iRegP ary, iRegI cnt, iRegI result, immI basic_type,
                         iRegLNoSp tmp1, iRegINoSp tmp2,
                         iRegINoSp tmp3, iRegLNoSp tmp4, rFlagsReg cr)

or
(2) [ iRegPNoSp/iRegINoSp ]

instruct arrays_hashcode(iRegPNoSp ary, iRegINoSp cnt, iRegINoSp result, immI basic_type,
                         iRegLNoSp tmp1, iRegINoSp tmp2,
                         iRegINoSp tmp3, iRegLNoSp tmp4, rFlagsReg cr)

it's impossible to use '_USE_KILL_' hint in '_effect_' directive because AD compilation fails:

Building target 'images' in configuration 'linux-riscv64-server-release'
/jdk/src/hotspot/cpu/riscv/gc/z/z_riscv.ad(10306) Syntax Error: :In arrays_hashcode only bound registers can be killed: iRegP ary

or

Building target 'images' in configuration 'linux-riscv64-server-release'
/jdk/src/hotspot/cpu/riscv/gc/z/z_riscv.ad(10306) Syntax Error: :In arrays_hashcode only bound registers can be killed: iRegPNoSp ary


IMHO, the usage of '_USE_KILL_' for _ary_/_cnt_ looks reasonable since
we actually do use/modify them both in the assembler body, and avoiding any
hint or usage other hint ('_USE_'?) may be wrong here even if that does
not cause a failure in pre-integration tests. It is interesting that many
intrinsics' definitions in AD files similarly use "concrete" registers in
X86/RISC-V archs, possibly due to the mentioned "_only bound registers can be
killed_" ADLC requirement.

I'd like to mention that applying '_USE_KILL_' to 'result' causes the error
in runtime:

---------------  T H R E A D  ---------------

Current thread (0x0000003fc0189130):  JavaThread "C2 CompilerThread0" daemon [_thread_in_native, id=54524, stack(0x0000003f7bc00000,0x0000003f7be00000) (2048K)]


Current CompileTask:
C2:828  117       4       java.lang.String::hashCode (60 bytes)

Stack: [0x0000003f7bc00000,0x0000003f7be00000],  sp=0x0000003f7bdfbba0,  free space=2030k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x5e1ad2]  Node_Backward_Iterator::next()+0x106
V  [libjvm.so+0x5e4040]  PhaseCFG::global_code_motion()+0x242
V  [libjvm.so+0x5e5266]  PhaseCFG::do_global_code_motion()+0x38
V  [libjvm.so+0x455142]  Compile::Code_Gen()+0x198
V  [libjvm.so+0x458168]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0xce4
V  [libjvm.so+0x3a239a]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x278
V  [libjvm.so+0x45d11e]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0x804
V  [libjvm.so+0x45f9e4]  CompileBroker::compiler_thread_loop()+0x4f0
V  [libjvm.so+0x6768b4]  JavaThread::thread_main_inner() [clone .part.0]+0x98
V  [libjvm.so+0xb1d3ea]  Thread::call_run()+0x8c
V  [libjvm.so+0x97253c]  thread_native_entry(Thread*)+0xd0
C  [libc.so.6+0x6a51c]
C  [libc.so.6+0xb7e3e]

siginfo: si_signo: 11 (SIGSEGV), si_code: 1 (SEGV_MAPERR), si_addr: 0x0000000000000028

and similarly I do not see in any intrinsics around the usage of '_effect_' hint for '_result_',
again both for X86/RISC-V archs.

I suggest the definition which looks "in sync" with X86 version and similar RISC-V
intrinsics:

instruct arrays_hashcode(iRegP_R11 ary, iRegI_R12 cnt, iRegI_R10 result, immI basic_type,
                         iRegLNoSp tmp1, iRegINoSp tmp2,
                         iRegINoSp tmp3, iRegLNoSp tmp4, rFlagsReg cr)
%{
  match(Set result (VectorizedHashCode (Binary ary cnt) (Binary result basic_type)));
  effect(TEMP tmp1, TEMP tmp2, TEMP tmp3, TEMP tmp4,
         USE_KILL ary, USE_KILL cnt, USE basic_type, KILL cr);

As an alternative we can use:

instruct arrays_hashcode(iRegP ary, iRegI cnt, iRegI_R10 result, immI basic_type,
                         iRegLNoSp tmp1, iRegINoSp tmp2,
                         iRegINoSp tmp3, iRegLNoSp tmp4, rFlagsReg cr)
%{
  match(Set result (VectorizedHashCode (Binary ary cnt) (Binary result basic_type)));
  effect(TEMP tmp1, TEMP tmp2, TEMP tmp3, TEMP tmp4,
        USE basic_type, KILL cr);

or as third possibilitiy even introduce additional temp registers and copy
ary/cnt into them at the beginning. But I personally do not like these 2 variants.

Ideally we need to understand why other intrinsics (a) use "concrete" registers and
(b) do not use any hint for 'result' regs, but that look to me as a separate not-so-small
activity. :-(

What do you think? Could C2 regalloc experts give us more details for usage of concrete regs in
intrinsics' definitions?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16629#discussion_r1409186277