[aarch64-port-dev ] RFR: 8235385: AArch64: Crash on aarch64 JDK due to long offset

[Andrew Dinn]

On 12/12/2019 19:14, Andrew Haley wrote:
> In a rather belt-and-braces way I've also added some code that fixes
> up illegal addresses. I did consider removing it, but I left it in
> because it doesn't hurt. If we ever do generate similar illegal
> addresses, debug builds will assert. I'm not sure whether to keep this
> or not.
> 
> Andrew Dinn will probably have a cow when he sees this patch. :-)
> 
> OK for HEAD?
Well, that wasn't quite so painful as it sounded.

Code Review:

The repeated warning comments are a very good idea.

I spotted two things in aarch64.ad:

1) Range rule:

7115 // Load Range
7116 instruct loadRange(iRegINoSp dst, memory8 mem)
7117 %{
7118   match(Set dst (LoadRange mem));
7119
7120   ins_cost(4 * INSN_COST);
7121   format %{ "ldrw  $dst, $mem\t# range" %}
7122
7123   ins_encode(aarch64_enc_ldrw(dst, mem));
7124
7125   ins_pipe(iload_reg_mem);
7126 %}

I think that should be memory4? Also, should you maybe change the comment to

  // Load Range (32 bit signed)


2) Pop count rules

The following rule uses 'memory4' to declare the memory op but passes
'sizeof (jfloat)' to the ldrs call. Would the latter not be better
passed as 4? (n.b. you use the matching numeric literal as argument to
loadStore in the encoding class definitions).

8161 instruct popCountI_mem(iRegINoSp dst, memory4 mem, vRegF tmp)
%{8162   predicate(UsePopCountInstruction);
8163   match(Set dst (PopCountI (LoadI mem)));
8164   effect(TEMP tmp);
8165   ins_cost(INSN_COST * 13);
8166
8167   format %{ "ldrs   $tmp, $mem\n\t"
8168             "cnt    $tmp, $tmp\t# vector (8B)\n\t"
8169             "addv   $tmp, $tmp\t# vector (8B)\n\t"
8170             "mov    $dst, $tmp\t# vector (1D)" %}
8171   ins_encode %{
8172     FloatRegister tmp_reg = as_FloatRegister($tmp$$reg);
8173     loadStore(MacroAssembler(&cbuf), &MacroAssembler::ldrs,
tmp_reg, $mem->opcode(),
8174               as_Register($mem$$base), $mem$$index, $mem$$scale,
$mem$$disp, sizeof (jfloat));
8175     __ cnt($tmp$$FloatRegister, __ T8B, $tmp$$FloatRegister);
8176     __ addv($tmp$$FloatRegister, __ T8B, $tmp$$FloatRegister);
8177     __ mov($dst$$Register, $tmp$$FloatRegister, __ T1D, 0);
8178   %}
  . . .

The same question applied for the next definition

8204 instruct popCountL_mem(iRegINoSp dst, memory8 mem, vRegD tmp) %{
  . . .

Yes, I too do not like the look of legitimize_address and it ought not
to be necessary, given that the assert in debug mode ought to stop us
hitting this in product mode. Still belt and braces is always a good
thing so I'm happy for to stay (and do no harm).

Testing:

Was there a rationale for picking those specific offsets for the
accesses? Anyway, I'm assuming the test didn't crash the JVM ;-)

So, the only real issue is the size in the Range rule. I guess you
didn't trigger a case where that mattered. Modulo that it is good to push.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill