This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Xtensa port, part 2: config.gcc and new config/xtensa files


Thanks for your suggestions.  See my comments/questions below:

Richard Henderson wrote:

> On Fri, Nov 30, 2001 at 11:31:36AM -0800, Bob Wilson wrote:
> 
>>+ #include <xtensa/xtensa.h>
>>
> 
> Use tm_file, not nested includes.


OK.  I knew I had to do this for 3.1 anyway.  I guess I might as well do 
it for the 3.0 version as well....



>>+ #if !XCHAL_HAVE_NSA
>>+ 	.section .rodata
>>+ 	.align	4
>>+ 	.type countLeadingZerosHigh,@object
>>+ countLeadingZerosHigh:	
>>
> 
> Looks like this gets pulled into _every_ lib1funcs.asm object
> file, not just the one or two that need it.  Ought to set .size
> as well.
> 
> Consider putting it in its own object file, giving it a name
> nicely in implementation space, and marking it .hidden.


Very good idea!  I hadn't noticed this problem, but even if I had, I 
wouldn't have known about ".hidden" if you hadn't pointed me to it.



>>+         .global _xtensa_libgcc_window_spill
>>+ 	.global	_xtensa_nonlocal_goto
>>+ 	.global	_xtensa_sync_caches

>

> These are not in the implementation namespace.  Use either __ or _[A-Z].


OK.  That's easy enough to fix.


> 
> 
>>--- gcc-3.0.2/gcc/config/xtensa/t-linux	Mon Nov 12 16:35:18 2001
>>
> [...]
> 
>>--- gcc-3.0.2/gcc/config/xtensa/t-xtensa	Mon Nov 12 16:35:18 2001
>>
> 
> What's different?


Nothing anymore.  Until recently we had some special settings for Linux, 
but they've converged.  I can't think of any reason for them to diverge 
again, so I'm removing t-linux.



>>--- gcc-3.0.2/gcc/config/xtensa/xm-xtensa.h	Mon Nov 12 16:35:18 2001
>>
> 
> I don't think you need anything in this file anymore.
> It should all be detected by autoconf.


I tried removing this but it breaks on the 3.0 branch (which is what I'm 
submitting these patches against).  I will remove it for the 
top-of-trunk version.


>>+ void
>>+ abort_with_insn (insn, reason)
>>
> 
> Eh?


I'm not sure what you mean here.  I copied this function from the MIPS 
port.  Is there some reason why it shouldn't be used?

 
>>+ 	    signed int val = INTVAL(op);
>>+ 	    val = (val << 20) >> 20;
>>
> 
> Not portable.  (1) Right shift of signed values are implementation
> defined, (2) sizeof(int) varies, (3) why wasn't this properly sign
> extended to begin with?


I don't know why this code was ever needed.  I'm going to remove it (and 
the similar code nearby) and then check to make sure nothing breaks.



>>+   /* Search all instructions, looking for the insn that sets up the
>>+      frame pointer.  This search will fail if the function does not
>>+      have an incoming argument in $a7, but in that case, we can just
>>+      set up the frame pointer at the very beginning of the
>>+      function. */
>>
> 
> Err, why would you be searching for this, rather than *knowing*
> where it is by virtue of having emitted it in the prologue expander?


It isn't emitted by the prologue expander.  The only way that I could 
find to make this work without adding new hooks into GCC was to emit the 
"unspec_volatile" as a side effect of copying the incoming argument in 
a7 (the hard frame pointer register) to a pseudo.  I could look at the 
number and types of the incoming arguments to determine if there is an 
argument in a7 but I'd still have to find the "unspec_volatile" insn 
since I need to transform all the instructions prior to that point. 
It's a lot easier to just search for it.



>>+ /* Set up the stack and frame (if desired) for the function.  */
>>+ 
>>+ void
>>+ xtensa_function_prologue (file, size)
>>
> 
> You should really consider doing prologue/epilogue in rtl.  At some
> point in the indefinite future we'd like to remove support for this
> as text...


OK.  Would it be acceptable to contribute the code as-is and promise to 
work on this?  If possible, I'd like to get this into a stable 3.0.x 
release.  The Xtensa code has been pretty stable for a while and I don't 
want to destabilize it now.


> 
> You don't define a target struct, so this isn't even going to compile
> against 3.1.


I've got a separate set of files for 3.1 and I've already added the 
target struct there.  I'll submit the 3.1 files once the 3.0 version is 
sorted out.



>>+ #define STATIC_CHAIN						\
>>+   gen_rtx_MEM (Pmode, plus_constant (stack_pointer_rtx, -5 * UNITS_PER_WORD))
>>
> 
> Your operating system implements a red-zone, right?


I sure hope so.  The Xtensa ISA reserves 4 words below the stack pointer 
anyway as a register window spill area, so this is just increasing that 
space by a bit.  I've think we've documented this requirement for 
everyone working on OS implementations for Xtensa.



>>+   char buffer[30];
>>+   int shift;
>>+   if (BITS_BIG_ENDIAN)
>>+     shift = (32 - (INTVAL(operands[2]) + INTVAL(operands[3]))) & 0x1f;
>>+   else
>>+     shift = INTVAL(operands[3]) & 0x1f;
>>+   sprintf(buffer, \"extui\\t%%0, %%1, %d, %d\",
>>+ 	  shift, INTVAL(operands[2]));
>>+   output_asm_insn(buffer, operands);
>>+   return \"\";
>>
> 
> Consider
> 
>    ...
>    operands[3] = GEN_INT (shift);
>    return "extui\t%0, %1, %3, %2";


OK.  There were a few other similar cases, and I fixed those up as well.




> All in all it looks fairly clean.


Good!  I'll resubmit this patch as soon as I finish sorting out the 
include file changes.

--Bob


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]