ARM: Volatile handlers

Wed Dec 7 02:18:05 PST 2011

2011-12-05 18:38, Andrew Haley skrev:
> I've rewritten all the volatile code.  The previous version I posted
> was buggy and unmaintainable.  I've rewritten it to duplicate every
> handler into a volatile and a non-volatile version.  This bloats the
> code rather, but it's more efficient and much easier to understand.
>
> Xerxes, please have a look.
>
> Thanks,
> Andrew.
>

Hi Andrew I have primed myself with Doug Leas JSR-133 cookbook
http://gee.cs.oswego.edu/dl/jmm/cookbook.html

I am still proofreading to check if the correct barrier are at the right location, so far the code looks reasonable.

I have found some issues with the new macros, comments are in-lined below.

> --- a/arm_port/hotspot/src/cpu/zero/vm/bytecodes_arm.def	Mon Nov 28 20:06:41 2011 +0000
> +++ b/arm_port/hotspot/src/cpu/zero/vm/bytecodes_arm.def	Mon Dec 05 12:33:28 2011 -0500
> @@ -1573,6 +1573,7 @@
>   	DISPATCH_NEXT
>   	SW_NPC	cmp	tmp1, #0
>   	SW_NPC	beq	null_ptr_exception_jpc_3
> +	GO_IF_VOLATILE	r3, tmp2, 3f
>   	ldr	tmp2, [tmp2, #CP_OFFSET+8]
>   	DISPATCH_NEXT
>   .abortentry78:
> @@ -1581,6 +1582,17 @@
>   	DISPATCH_NEXT
>   	PUSH	tmp2
>   	DISPATCH_FINISH
> +3:
> +	VOLATILE_VERSION
> +	ldr	tmp2, [tmp2, #CP_OFFSET+8]
> +	DISPATCH_NEXT
> +.abortentry78_v:
> +	ldr	tmp2, [tmp1, tmp2]
> +	dmb

Suggestion it would be a little clearer if we had put a new LoadLoad macro here instead of the dmb directly.
Such an change would allow us to better maintain the code in the future to take advantage of new less expensive barriers when they get introduced in newer ARM ISA,
for example make it trivial to update the code if ARM would introduce a new barrier similar to IA64 ld.acq.

>   (iputfield) iputfield {
> @@ -1669,6 +1731,7 @@
>   	DISPATCH_NEXT
>   	SW_NPC	cmp	tmp1, #0
>   	SW_NPC	beq	null_ptr_exception_jpc_3
> +	GO_IF_VOLATILE	r2, tmp2, 3f
>   	ldr	tmp2, [tmp2, #CP_OFFSET+8]
>   	DISPATCH_NEXT
>   	DISPATCH_NEXT
> @@ -1676,6 +1739,17 @@
>   .abortentry83:
>   	str	r3, [tmp1, tmp2]
>   	DISPATCH_FINISH
> +3:
> +	VOLATILE_VERSION
> +	ldr	tmp2, [tmp2, #CP_OFFSET+8]
> +	DISPATCH_NEXT
> +	DISPATCH_NEXT
> +	DISPATCH_NEXT
> +	dmb_st

Similar here we could use a StoreStore macro instead of dmb_st

> +.abortentry83_v:
> +	str	r3, [tmp1, tmp2]
> +	dmb

And here we could use a StoreLoad macro instead of the dmb

I think such use of new macros would make the code more portable and readable as well.

> @@ -1687,6 +1761,7 @@
> diff -r 0a0072170876 arm_port/hotspot/src/cpu/zero/vm/cppInterpreter_arm.S
> --- a/arm_port/hotspot/src/cpu/zero/vm/cppInterpreter_arm.S	Mon Nov 28 20:06:41 2011 +0000
> +++ b/arm_port/hotspot/src/cpu/zero/vm/cppInterpreter_arm.S	Mon Dec 05 12:33:28 2011 -0500
> @@ -585,6 +585,32 @@
>   	.fpu softvfp
>   #endif // HW_FP
>
> +#ifndef	__ARM_ARCH_7A__
Can we make this #ifndef armv8 future proof?

> +#	define dmb VOLATILE_BARRIER
> +#	define dmb_st VOLATILE_BARRIER
> +#else
> +#	define	dmb_st .word   0xf57ff05e
> +#endif

Please move this define section down below the VOLATILE_BARRIER define to prevent compile errors on non armv7 machines.

> +
> +	.macro	VOLATILE_BARRIER arg
> +	stmfd	sp!, {r2, lr}

> +	mov	r2, #0xfa0
better to use movw here?
> +	movt    r2, #0xffff
I would suggest to replace these two instructions with a single
	mov	r2, #0xffff0fa0 @ kernel_dmb

> +	blx	r2
> +	ldmfd	sp!, {r2, lr}
> +	.endm
> +	
> +	.macro	GO_IF_VOLATILE reg, cp_cache, label
> +	ldr	\reg, [\cp_cache, #CP_OFFSET+CP_CACHE_FLAGS]
> +	tst	\reg, #(1<<CP_CACHE_VOLATILE_FIELD_FLAG_BIT)
> +	bne	\label
> +	.set	dispatch_saved, dispatch_state
> +	.endm

Here it would be nice with a comment, something like:
	@ the macros used by the non-volatile version in between
	@ the GO_IF_VOLATILE and VOLATILE_VERSION will
	@ alter the dispatch_state, save it!

> +	.macro VOLATILE_VERSION
I would suggest to add a sanity check at the start of this macro:
	.if dispatch_state == 0
> +	.set	dispatch_state, dispatch_saved
	.else
	.error "VOLATILE_VERSION macro used before non-volatile DISPATCH_FINNISH."
	.endif
> +	.endm

> --- a/arm_port/hotspot/src/cpu/zero/vm/thumb2.cpp	Mon Nov 28 20:06:41 2011 +0000
> +++ b/arm_port/hotspot/src/cpu/zero/vm/thumb2.cpp	Mon Dec 05 12:33:28 2011 -0500
> @@ -27,7 +27,7 @@
>
>   #define T2EE_PRINT_COMPILATION
>   #define T2EE_PRINT_STATISTICS
> -//#define T2EE_PRINT_DISASS
> +#define T2EE_PRINT_DISASS
...

> -	  len = print_insn_little_arm((bfd_vma)codebuf+idx,&info);
> +	  len = opcodes.print_insn_little_arm((bfd_vma)codebuf+idx,&info);
>   	  if (len == -1) len = 2;
>   	  idx += len;
>   	}

OK! Super to have this debug disass support enabled by default without adding a new build dependency!

> @@ -7163,7 +7242,7 @@
>     u32 loc_irem, loc_idiv, loc_ldiv;
>     int rc;
>
> -  if (!(CPUInfo&  ARCH_THUMBEE)) {
> +  if (!(CPUInfo&  ARCH_THUMBEE) || !UseCompiler) {
>       DisableCompiler = 1;
>       return;
>     }
Nice. i assume this will make -Xint work to disable the t2jit.

I will post a follow up code-review when i have proof-read the placement of all the barriers in the .S ASM interpreter, the .def bytecode defines and the .cpp thumb2 jit according to the JSR-133 cookbook.

Cheers
Xerxes