ARM: Volatile handlers

[Andrew Haley]

On 12/07/2011 10:18 AM, Xerxes Rånby wrote:
> 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.

That's never going to happen.  The ARM already has eight different
kinds of memory barrier.

>> +.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.

Portable?  :-)

OK, I'll give this idea some thought.

>> @@ -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?

I'd love to, but I do not know what macros GCC developers are going to
use.

>> +#	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.

I don't understand this comment.

>> +
>> +	.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

Pardon?  What ARM are you using?  :-)

>> +	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!

OK.

>> +	.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

OK.

>> @@ -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.

It does.

Thanks,
Andrew.