RFA: Exhume the ARM port [PR icedtea/484, 323]

[Andrew Haley]

On 10/21/2011 12:21 AM, Xerxes Rånby wrote:
> 2011-10-20 15:55, Andrew Haley skrev:
>  > This patch is in two parts.  The first part simply undoes the commit
>  > that deleted the ARM assembler port.  The second part patches the
>  > port to work with the Hard FP ABI and HotSpot 20.
> 
> Thank you Andrew for showing us that it indeed are possible for a polymath to maintain the 
> ARM assembler port!
> 
> sorry for the long reply, not every day you review a 2000line patch. ;)
> 
>  >
>  > This is the first patch.  As it simply undoes an earlier patch, I don't
>  > include a diff.
>  >
>  > 2011-09-15  Andrew Haley<aph at redhat.com>
>  >
>  > 	Reinstate the ARM assembler port.  Back out this patch:
>  >
>  >    2011-07-11  Xerxes RÃ¥nby<xerxes at zafena.se>
>  >
>  >            Removal of the ARM assembler port, unbreaks Zero and Shark builds.
> OK, as long as the second part below gets updated and pushed at the same time!
> 
>  >
>  > This second patch is the update.  I'm sorry it's rather long, but
>  > anything much smaller crashes during bootstrap; you really need it
>  > all.  It's still only JDK 6 at the moment.  I'll look at implementing
>  > invokedynamic next, and there are some performance improvements on the
>  > way.
>  >
>  > OK for trunk?
> My main concern with the second patch, as is, are that the pr323 update will not work in 
> combination with the Hotspot profiler. In order to make the profiler happy we need to 
> change to code to first set THREAD_LAST_JAVA_SP to 0 before THREAD_LAST_JAVA_FP can be 
> updated.

OK.

> I have posted parts on how Rob Savoye tried to restructure the code to make the profiler 
> happy extracted from: http://www.senecass.com/projects/OpenJDK-ARM/thumb2-090710.patch 
> (this patch do not include any HS20 fixes since HS20 had not been released when that patch 
> was made).  so if you see ... : in the comments then it means the block above could be 
> rewritten as the following block below to make the profiler happy...


> The second patch contains the whole file for patches/arm.patch instead of the diff.

Ah OK, that must be because of the way I made the diff from Mercurial.

>  >
>  > 	PR icedtea/484:
>  > 	* arm_port/hotspot/src/cpu/zero/vm/cppInterpreter_arm.S
>  > 	(fast_empty_entry, normal_entry_synchronized, normal_entry):
>  > 	Return 0, #deoptimized_frames.

> Why are the Return 0 unneeded for native_entry and native_entry_synchronized?

They didn't seem to be necessary.  I will add them just in case.

>  > 	* arm_port/hotspot/src/cpu/zero/vm/thumb2.cpp (Thumb2_Return):
>  > 	Likewise.
> 
> Question: Can the thumb2 jit deoptimize a frame?

No, I don't think it can.

> These changes looks fine. look out for Makefile.am chunks that are already found in 
> icedtea6 HEAD. More details in the patch below:

Yes, of course.

>  > 	Update to HS20:
>  > 	* arm_port/hotspot/src/cpu/zero/vm/thumb2.cpp: Replace all calls
>  > 	to fatal1() (which no longer exists) with calls to fatal().
>  > 	(Thumb2_is_zombie, Thumb2_pass2): invoke* and {get,put}* now take
>  > 	native, not Java, byte ordered indexes.
>  > 	(Thumb2_disass, Thumb2_codegen, Thumb2_tablegen):
>  > 	Bytecodes::special_length_at() takes different arguments from the
>  > 	previous version; adjust suitably.
>  > 	* arm_port/hotspot/src/cpu/zero/vm/cppInterpreter_arm.S: Replace
>  > 	call to report_fatal_vararg(char const*, int, char const*, ...)
>  > 	(which no longer exists) with call to Helper_report_fatal.
> 
> Can you add a note about the fixed ISTATE_SELF_LINK reference?

OK.

>  > 	* arm_port/hotspot/src/cpu/zero/vm/bytecodes_arm.def (new):
>  > 	Renumber the fast bytecodes iload_0_iconst_N to iload_3_iload_N.
>  > 	* arm_port/hotspot/src/cpu/zero/vm/thumb2.cpp: Likewise.
>  > 	* patches/arm.patch: Likewise.
>  > 	Add *.S to the list of source files.
>  > 	* arm_port/hotspot/src/cpu/zero/vm/asm_helper.cpp (All #includes)
>  > 	: Move to new OpenJDK include file format.
>  > 	(Helper_report_fatal): New assember helper.
>  > 	(print_vm_offsets): Add THREAD_LAST_JAVA_FP.
> 
> Please mention the add of SIZEOF_FFI_CIF to this file in the changelog.

OK.

>  > 	Hard FP port:
>  > 	* arm_port/hotspot/src/cpu/zero/vm/thumb2.cpp (Thumb2_Initialize):
>  > 	Add hard FP variants for the stubs that need it.
>  > 	* arm_port/hotspot/src/cpu/zero/vm/cppInterpreter_arm.S
>  > 	(POPF0, POPF1, POPD0, POPD1,PUSHF0, PUSHD0): New macros.
>  > 	(.eabi_attribute): Use real names.  Add declaration for hard
>  > 	FP ABI.
>  > 	(.fast_native_return_double, .fast_native_return_float, .fast_copy_double, 
> .fast_copy_float): New.
>  > 	(fast_native_entry): use SIZEOF_FFI_CIF, not 24.
> 
> Nice! i have been wondering what this magic constant was refering to.
>
>  > 	Add logic to handle args in FP registers.
>  > 	(FIND_LOWEST_BIT, FIND_LOWEST_BIT_PAIR, COPY_DOUBLE, COPY_FLOAT): New macros.
>  > 	(.copy_float_table, .copy_double_table): New.
>  > 	* arm_port/hotspot/src/cpu/zero/vm/bytecodes_arm.def
>  > 	(frem, drem, f2i, f2l, d2i, d2l): Add hardfp args.
>  >
>  > 	* openjdk-ecj/hotspot/src/cpu/zero/vm/cppInterpreter_arm.S
>  > 	(REWRITE_PAIRS): New macro.
>  > 	* openjdk-ecj/hotspot/src/cpu/zero/vm/bytecodes_arm.def: Use
>  > 	REWRITE_PAIRS to prevent rewriting pairs of bytecodes in the
>  > 	instruction stream.
>  >
>  > 	* patches/arm-debug.patch: New file.
>  >
> 
>  > diff -r 886f06403d1c Makefile.am
>  > --- a/Makefile.am	Wed Oct 19 12:37:40 2011 -0400
>  > +++ b/Makefile.am	Thu Oct 20 09:35:07 2011 -0400
> ...
>  > @@ -650,7 +652,7 @@
>  >    clean-icedtea-against-ecj clean-extract-ecj clean-generated clean-replace-hotspot \
>  >    clean-rewriter clean-rewrite-rhino clean-rt clean-bootstrap-directory \
>  >    clean-bootstrap-directory-ecj clean-bootstrap-directory-symlink \
>  > - clean-bootstrap-directory-symlink-ecj clean-fonts jtregcheck
>  > + clean-bootstrap-directory-symlink-ecj clean-fonts
>  >   	if [ -e bootstrap ]; then \
>  >   	  rmdir bootstrap ; \
>  >   	fi
> 
> This chunk are already fixed in HEAD:
> http://icedtea.classpath.org/hg/icedtea6/rev/6b8691d033ae27da89ac605fca75e15a0b426500

Yes, I know.  We crossed over during testing.

It's impossible to produce a properly tested patch that is correct against HEAD.
You'd have to update again, and that would mean you have to build, and then
it would change.  Lather, rinse, repeat...  :-)

> Nice, thank you for fixing this micro optimization and make the ISTATE struct valid.
> 
>  > @@ -886,12 +972,17 @@
>  >   	blt	.fast_native_entry_throw_stack_overflow
>  >   	cmp	r5, #0
>  >   	bne	.fast_native_entry_got_handleraddr
> 
> I think this code are broken.
> you need to add
> 
>          str     r5, [r9, #THREAD_LAST_JAVA_SP] @ r5 is zero at this point
> 
> here to clear THREAD_LAST_JAVA_SP before updating THREAD_LAST_JAVA_FP

Another profiler fix, I take it.  OK.

> How about rewriting this chunk with:
> @@ -2355,7 +2416,16 @@
>   	add	r0, r0, ip
>   	str	tmp_vvv, [tmp1, #THREAD_TOP_ZERO_FRAME]
>   @	CACHE_JPC
> -	str	tmp_vvv, [tmp1, #THREAD_LAST_JAVA_SP]
> +@	set SP to zero before setting the FP
> +	mov	r2, #0
> +	str	r2, [tmp1, #THREAD_LAST_JAVA_SP]
> +@	set FP to the top zero frame	
> +	str	tmp_vvv, [tmp1, #THREAD_LAST_JAVA_FP]
> +@	get the stack pointer, use r3 as it gets reset below
> +	ldr	r3, [tmp1, #THREAD_JAVA_SP]
> +@	set SP to the current top of stack
> +	str	r3, [tmp1, #THREAD_LAST_JAVA_SP]
> +	
>   	add	dispatch, r1, r0
>   	ldr	r0, [istate, #ISTATE_METHOD]
>   	ldr	r3, [r0, #METHOD_ACCESSFLAGS]

OK.

> I think there are a typo here:  "ilaod_iload_N"  instead of "iload_iload_N"

Oh yes.  :-)

My, you are very thorough...

Andrew.