[8u] RFR: cumulative patch required for C1 integration

[Sergey Nazarkin]

Hi Ed,

thanks for review. I’ll cleanup build warnings and, as side-effect, prepare patch for JDK9. 

My plan is to get rid of confusing  #if 0/1 macroses keeping in mind your comment regrading large pages support.  

Regarding stubGenerator_aarch32.cpp. Both pieces of code are functionally equals since callee saved registers are not supported for now. Actually we forgot to backport  the patch to our workspace and fix the issue ourself. I’ll revert modification in the next changeset. 

Renaming of instruction size constant is part of refactoring NativeInsatruction classes. We reuse it  to express dependency of various instruction from base constant.  

With best regards,

Sergey Nazarkin




> On 23 Feb 2016, at 18:00, Edward Nevill <edward.nevill at gmail.com> wrote:
> 
> On Wed, 2016-02-17 at 15:10 +0000, Sergey Nazarkin wrote:
>> Hi!
>> 
>> Azul team continues its work on AArch32 port. We’ve achieved some results. 
>> To facilitate future synchronization, we would like to share our work  that touches non-JIT part of the code base. 
>> http://cr.openjdk.java.net/~snazarki/preC1/
>> 
>> 
>> Sergey Nazarkin
> 
> Hi Sergey,
> 
> I have tested this on jdk8u and jdk9 and it looks fine.
> 
> I have a few comments on the patch, mostly cosmetic.
> 
> --- In src/cpu/aarch32/vm/assembler_aarch32.cpp
> 
> There seem to be a few more compiler warnings generated by this patch. We should aim to get the no. of compiler warnings down to 0. This is not so much of an issue with 8, but 9 by default builds with warnings treated as errors so I have to build with --disable-warnings-as-errors
> 
> Here are a couple. It would be helpful if you could fix any more you come across.
> 
> +bool Address::is_safe_for(InsnDataType type) {
> +  switch (_acc_mode) {
> +    case imm:
> +    case lit:
> +      return offset_ok_for_immed(_offset, type);
> +    case reg:
> +      return shift_ok_for_index(_shift, type);
> +    case no_mode:
> +    default:
> +      ShouldNotReachHere();
> +  }
> }
> 
> Needs a 'return false' at the end to prevent the warning "No return from value generating function"
> 
> --- In src/cpu/aarch32/vm/interpreter_aarch32.cpp
> 
> -void InterpreterGenerator::generate_transcendental_entry(AbstractInterpreter::MethodKind kind, int fpargs) {
> -  address fn = NULL;
> +void InterpreterGenerator::generate_transcendental_entry(AbstractInterpreter::MethodKind kind) {
> +  address fn;
> 
> 
> The 'fn = NULL' here was to prevent the warning 'Variable may be used uninitialised'.
> 
> --- In src/cpu/aarch32/vm/relocInfo_aarch32.cpp
> 
> 2 occurrences of
> 
> +#if 0
> +    warning("TODO: poll_Relocation::fix_relocation_after_move: "
> +    "ensure relocating does nothing on relative instruction");
> +#endif
> 
> I think this can be replaced with
> 
>  assert(false, "TODO: ...");
> 
> or delete it if this is not in fact the case.
> 
> --- In src/cpu/aarch32/vm/nativeInst_aarch32.hpp
> 
> -  enum { instruction_size = 4 };
> +  enum { arm_insn_sz = 4 };
> 
> instruction_size seems to have changed throughout to arm_insn_sz
> 
> Is there a reason for this? Do you plan to add a 'thumb_insn_sz' at some point?
> 
>  // TODO remove this, every command is 4byte long
> #if 1
> 
> I think we should just leave the methods in anyway. They are doing no harm and will be inlined in any case. I think the comment and the #if 1 should be deleted.
> 
> --- In src/cpu/aarch32/vm/macroAssembler_aarch32.cpp
> 
> void MacroAssembler::far_call(Address entry, CodeBuffer *cbuf, Register tmp) {
>   assert(CodeCache::find_blob(entry.target()) != NULL,
>          "destination of far call not found in code cache");
> +  // TODO performance issue: if intented to patch later,
> +  // generate mov rX, imm; bl rX far call (to reserve space)
> +#if 0
>   if (far_branches()) {
> +#else
> +  if (entry.rspec().type() != relocInfo::none || far_branches()) {
> +#endif
> 
> 
> All the far_call and far_branch and trampoline code was to support code caches larger than 128M on aarch64. This was particularly necessary for C2 with tiered compilation where the code cache could grow quite large.
> 
> I think we should strip all of this back out. If we do need to support large code caches in the future then we should engineer it back in.
> 
> This will be a better approach than trying to carry the code in the aarch32 port where it will rust and break in any case.
> 
> However for the purpose of this patch I think it can be left as is. Just another item on the TODO list:-)
> 
> --- src/cpu/aarch32/vm/stubGenerator_aarch32.cpp
> 
> -      rbp_off = frame::arg_reg_save_area_bytes/BytesPerInt,
> +      rfp_off = 0,
> 
> -    // lr and fp are already in place
> -    assert(frame::arg_reg_save_area_bytes == 0, "please modify this code");
> -    // __ sub(sp, rfp, frame::arg_reg_save_area_bytes + wordSize); // prolog
> +    assert(is_even(framesize), "sp not 8-byte aligned");
> 
> This seems to back out my change
> 
> http://cr.openjdk.java.net/~enevill/8148355/webrev/hotspot.changeset
> 
> which was discussed on the list
> 
> http://mail.openjdk.java.net/pipermail/aarch32-port-dev/2016-January/000078.html
> 
> Was this intentional?
> 
> 
> 
> Otherwise this patch looks fine. Could you please prepare a new patch taking into account my comments above and I will push it.
> 
> All the best,
> Ed.
> 
>