RE: CR16 Port addition

[Sumanth Gundapaneni <Sumanth dot Gundapaneni at kpitcummins dot com>]

Hi Richard and Joseph,

Thanks for your detailed review and valuable suggestions.

Please find attached the patch "cr16-gcc.patch" which contains modifications 
as suggested by Richard in his previous mail.

For your kind information, I am providing the recent posts regarding 
CR16 patches.
Initial post : http://gcc.gnu.org/ml/gcc-patches/2010-12/msg01676.html
Second post  : http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00803.html  

Please review the patch and let me know if there should be any 
modifications in it.

Richard, please go through my explanation for your comments.

>> +targetm = TARGET_INITIALIZER;
>I find it much cleaner to move the target initializer to the end 
I moved this to the end of the file as advised.

>> +  if ((rclass) == SHORT_REGS || (rclass) == DOUBLE_BASE_REGS \
>Trailing \.  Left over from macro-to-function conversion?
The trailing \ has been removed in the current patch.

>> + flag_split_wide_types = 0;
>I would really like to know more about this.
This particular flag causes the compiler to generate an ICE in 
gen_rtx_SUBREG. The subreg rtx with offset byte "2" is the reason 
for the issue. Though I disabled this flag I am hitting this ICE in 
dejagnu regression for a specific test case named 
"gcc.c-torture/compile/ptr-conv-1.c". Hacking at backend specifically 
in predicates doesn't seem to solve my problem. I believe this issue 
is due to the variable width registers in CR16 port 
(r0-r11 are 16-bit wide register, r12-r15 are 32-bit wide registers). 
Any hints on solving this problem would be much appreciated. 

>> +    flag_gcse = 0;
>Why?  It works for other targets.
This is probably related to SUBREG issue mentioned above.

>> +cr16_compute_frame (void)
>I would be very much happier if these functions did not store their 
>results into
I defined a structure "cr16_frame_info" to store these variables.

>> +if ((mode == Pmode) && (regno == 11))
>Is there a symbolic name for 11?
There is no symbolic name for register number 11. 
We will add a definition for readability.

>> +  last_parm_in_reg = 0;
>Using a global here is definitely wrong.  
I defined this variable in CUMULATIVE_ARGS structure as per your suggestion.

>> +      if ((next_param == (tree) 0) && (TREE_VALUE (param) !=
>s/(tree) 0/NULL_TREE/g
I am not sure what needs to be modified here.

>I don't see a constraints.md file?
I added the constraints.md file and deleted the traditional macros.

>> +  [(set (match_operand:SHORT 0 "bit_operand" "=mZ")
>The constraint here is almost certainly wrong.  It should be only "=Z".
This has been modified.

 
>> +(define_predicate "reg_operand"
>This is wrong.  This issue should be handled with 
>CANNOT_CHANGE_MODE_CLASS
This predicate definition has been deleted and changes have been made in patterns 
to avoid common subreg issues.

>> +(define_expand "adddi3"
>You should trust the predicates to do the right thing. 
This pattern has been modified and defined in a normal manner. 
Please verify the same.


>> +	(plus:SI (mult:SI (sz_xtnd:SI (match_operand:HI 1 "reg_operand" 
>> +"%r"))
>% should be used only when the constraints of the two operands are different.
This is rectified.

>> +  [(set (match_operand:HI 0 "reg_operand" "=r,=r")
>Extra = in operand 0.
This is rectified.

>> +(define_expand "andhi3"
>What is this for?  Disallowing subregs of pseudos?!?
This pattern has been adjusted correctly. Please verify the same.


>> +  "andd\t%T2,%T0\;andd\t%U2,%U0"
>You'd be better off without this, and let the compiler generate it 
>itself from two SImode ands.
This is handled with rtx. Please verify the same.


>> +(define_insn "*mov<mode>_double"
>This whole pattern has got to get cleaned up.  
I am planning to solve this once I am finished with subreg issues.

>> +(define_insn "*cmp<mode>_insn"
>I'm not sure I understand the logic here.  If you have a cbranch insn, 
>which you never split
This compare pattern is for scond instruction which you can find 
in the updated patch.

>> +(define_insn "indirect_jump_return"
>Extra parallel.  It's implicit in define_insn.
>> +(define_expand "call"
>These can be trivially merged.
Yes, I plan to work on them after fixing the subreg issues. 

>> +;;  Nop
>Nop really needs to output something.  
This has been fixed as suggested.

>> +;; Shared FLAT
> Huh?
These patterns are to support the target option MID_SHARED_LIBRARY.
I will enable this target option soon.  
 

gcc/ChangeLog:
--------------
2011-04-08	Sumanth G <sumanth.gundapaneni@kpitcummins.com>
		Jayant R Sonar <jayant.sonar@kpitcummins.com>
		
	* config.gcc: Add cr16-* support.

	* doc/extend.texi: Document cr16 extensions.
	* doc/install.texi: Document cr16 install.
	* doc/invoke.texi: Document cr16 options.
	* doc/md.texi: Document cr16 constraints.

	* config/cr16/cr16.c: New file.
	* config/cr16/cr16.h: New file.
	* config/cr16/cr16.md: New file.
	* config/cr16/cr16.opt: New file.
	* config/cr16/cr16-libgcc.s: New file.
	* config/cr16/cr16-protos.h: New file.
	
	* config/cr16/divmodhi3.c: New file.
	* config/cr16/predicates.md: New file.
	* config/cr16/constraints.md: New file.
	* config/cr16/t-cr16: New file.
	* config/cr16/unwind-dw2-cr16.c: New file.
	* config/cr16/unwind-dw2.h: New file. 

libgcc/ChangeLog:
-----------------
2011-04-08  Sumanth G <sumanth.gundapaneni@kpitcummins.com>
		Jayant R Sonar <jayant.sonar@kpitcummins.com>

	* config.host: Add National Semiconductor CR16 target (cr16-*-*).
	* config/cr16/crti.S: New file.
	* config/cr16/crtlibid.S: New file.
	* config/cr16/crtn.S: New file.
	* config/cr16/t-cr16: New file.
		
libstdc++-v3/ChangeLog:
-----------------------
2011-04-08  Sumanth G <sumanth.gundapaneni@kpitcummins.com>
		Jayant R Sonar <jayant.sonar@kpitcummins.com>

	*configure: Add National Semiconductor CR16 target (cr16-*-*).		
	*crossconfig.m4 : Add support for National Semiconductor CR16 target.

ChangeLog:
----------
2011-04-08  Sumanth G <sumanth.gundapaneni@kpitcummins.com>
		Jayant R Sonar <jayant.sonar@kpitcummins.com>

	* configure.ac: Add National Semiconductor CR16 target.
	* configure: Regenerate
   
Thanks in advance,
Sumanth G,
PUNE (India).

Attachment: cr16-gcc.patch
Description: cr16-gcc.patch