Re: ARM patches applied

[Richard Earnshaw <rearnsha at arm dot com>]

> Hi Richard,
> 
> : Sorry, but I think some of this is incomplete or just plain wrong.
> 
> *Sigh* You are right of course.  Here are some answers:
> 
> : > 	(CONDITIONAL_REGISTER_USAGE): Allow r10 to be used if stack
> : > 	checking is not enabled.
> : 
> : RISCiX configurations must not use r10 at any time.  How does this 
> : interact with use of r10 as the PIC register when stack checking is off.
> 
> Badly.  The patch below should correct this, although I have no real
> way to test it.
> 

Hmm, looking at your suggested patch, I think a better way is to create a 
SUBTARGET_CONDITIONAL_REGISTER_USAGE which can be defined in riscix.h, and 
is added to the end of CONDITIONAL_REGISTER_USAGE; this is more general 
than trying to fix things up with a TARGET_IS_RISCIX specific hack.  Also, 
I think the reg[10] bit in FIXED_REGISTERS and CALL_USED_REGISTERS should 
be cleared, and then set explicitly in CONDITIONAL_REGISTER_USAGE when the 
options require its reservation.


> : > 	(RETURN_IN_MEMORY): Always call arm_return_in_memory.
> : 
> : Does this maintain binary compatibility on NetBSD?
> 
> Yes.  netbsd.h undefines RETURN_IN_MEMORY so it was never being used
> anyway.

See below (arm_return_in_memory).  As a note here, I don't like the way 
the netbsd port is doing that (it uses non-pcc struct returning to compile 
just one file in the standard library which contains the soft-float code, 
it then needs structures of 2 words to be returned in registers -- very 
dodgy, it would be better to fix the source file in libc so the undef 
wasn't needed).

> 
> : > 	(arm_is_strong): New variable: true iff the target processor is a 
> : > 	StrongARM.
> : 
> : What about arm8/arm9 support.  These also have load scheduling.
> 
> This is accounted for by the LD_SCHED flag.

Sorry, I missed this since there was no mention of the ld_sched flag 
change in the ChangeLog.  This is ok.  However, in load_multiple_sequence 
you use this flag in a strictly incorrect manner (though it achieves the 
desired result with current processors).  The reason two ldr insns become 
faster has more to do with the fact that the ARMs which also support load 
scheduling also are able to do more than one cache access in a single 
cycle (arm9 and strongarm have Harvard caches, while arm8 has a double 
bandwidth cache).  This means that these cores can do both an instruction 
fetch and a data fetch in a single cycle, so the trick of calculating the 
address into a scratch register (one of the result regs) and then doing a 
load multiple actually becomes slower (and no smaller in code size).  That 
is the transformation

	ldr	rd1, [rbase + offset]
	ldr	rd2, [rbase + offset + 4]

to

	add	rd1, rbase, offset
	ldmia	rd1, {rd1, rd2}

produces worse code -- '3 cycles + any stalls on rd2' instead of '2 cycles 
+ any stalls on rd2'.  On ARMs with only one cache access per cycle, the 
first sequence could never complete in less than 6 cycles, whereas the ldm 
sequence would only take 5 and would make better use of sequential 
accesses if not hitting the cache.
	
I think this deserves a comment to clarify.

> 
> : > 	(arm_return_in_memory): Improve handling of structures.
> : 
> : Is this backwards compatible?
> 
> Not strictly.  With the new version empty structures will now be
> passed in a register rather than in memory.  I had missed float fields
> in unions however, so the patch below corrects this.

Ah, yes.  But what about Linux. 

Phil, is this likely to break anything?  Or do you want to move to full 
APCS compliance and declare anything outside of that as a bug.

> 
> : > 	* config/arm/arm.md: Remove "cpu" attribute.  Replace with
> : > 	"is_strongarm" and "is_arm_6_or_7" attributes.
> : 
> : ARM8/ARM9?
> 
> Allowed for.

OK.

> 
> : > 	(zero_extendhisi2): Check for TARGET_SHORT_BY_BYTES before
> : > 	arm_arch4.
> : > 	(extendhisi2):  Check for TARGET_SHORT_BY_BYTES before arm_arch4. 
> : 
> : This is just wrong.  TARGET_SHORT_BY_BYTES means don't use ldr to access a 
> : short because the MMU will trap if it isn't aligned.  On a machine with 
> : ldrh this doesn't apply so there is no need to synthesise a short access 
> : with byte loads.
> 
> Sorry - the patch fixes this as well.

This is I've been thinking about this one for a while.  Most of the 
confusion (you aren't the first) stems from the naming of the flag.  
Perhaps a better and more descriptive name for it would be 
-mmu-enforced-alignement.  The documentation also needs clarification 
whether we change the flag name or not.  Do you have any thoughts.

> 
> Richard Henderson has pointed out that my replacement of one attribute
> in the md file "cpu" with several different ones is really a backward
> step.  What I wanted to achieve was the placement of all the
> architecture/processor variation information into one place (the
> 'flags' field of the 'processors' structure), but I have not come up
> with a simple way to test this information using attributes in the .md
> file.

While Richard is correct.  Your change should lead to a much simpler 
atrributes file (the one that was previously generated contained some very 
long if clauses that checked the cpu combinations).  I think the jury is 
still out on this one.

> 
> Anyway I hope that this supplimental patch makes things a little bit
> better.  If you have no comments then I will apply it tomorrow.
> 
> 
> Cheers
> 	Nick
> 
> Tue Feb 23 14:54:14 1999  Nick Clifton  <nickc@cygnus.com>
> 
> 	* config/arm/riscix.h (TARGET_IS_RISCIX): Define to 1.
> 	* config/arm/riscix1-1.h (TARGET_IS_RISCIX): Define to 1. 
> 	* config/arm/arm.h (TARGET_IS_RISCIX): If not defined, define
> 	to 0.
> 	(CONDITIONAL_REGISTER_USAGE): Do not unfix r10 if
> 	TARGET_IS_RISCIX is true.
> 
> 	* config/arm/arm.c (return_in_memory): Float fields in unions
> 	force a return in memory.
> 
> 	* config/arm/arm.md (zero_extendhisi2): Undo previous change.
> 	(extendhisi2): Undo previous change.
>