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: [PATCH] add insn implementing signbit to middle end and s390


Wolfgang Gellerich wrote:

> 	* optabs.h: added declaration for signbit_optab.
Should start with captital letter:  "Added declaration ..."
In fact, should call out the changed element
	* optabs.h (signbit_optab): Add declaration.

> 	* s390.h.c: Added named constants for bit masks controllong the test
> 	  data class instruction
Typo in file name and "controllong".  Sentence should end in '.'.  Files
in subdirectories should be specified with relative path: config/s390/s390.h.
Also, the added constants should be listed in parenthesis, like so:

	* config/s390/s390.h (s390_TDC_positive_Zero): New constant.
	(s390_TDC_negative_Zero): Likewise.
	(s390_TDC_positive_normalized_Number): Likewise.
	(s390_TDC_negative_normalized_Number): Likewise.
	(s390_TDC_positive_denormalized_Number): Likewise.
	(s390_TDC_negative_denormalized_Number): Likewise.
	(s390_TDC_positive_Infinity): Likewise.
	(s390_TDC_negative_Infinity): Likewise.
	(s390_TDC_positive_quite_NaN): Likewise.
	(s390_TDC_negative_quite_NaN): Likewise.
	(s390_TDC_positive_signaling_NaN): Likewise.
	(s390_TDC_negative_signaling_NaN): Likewise.
	(s390_TDC_signbit_set): Likewise.

> 	* s390.c (s390_canonicalize_comparison): renamed CMPINT to CCU_TO_INT,
> 	  added a CCU-like optimization for CCU_TO_INT 
	This should be UNSPEC_..., the second CCU_TO_INT should presumably
	refer to UNSPEC_CCZ_TO_INT.

>  	* s390.md: added insns signbit_<mode>, TDC_insn_<mode>, 
> 	  ccz_to_int, renamed UNSPEC_CMPINT to UNSPEC_CCU_TO_INT, added 
>           UNSPEC_CCZ_TO_INT

The changed elements need to be specified, like so:

	* config/s390/s390.md ("signbit_<mode>"): New expander.
	("*TDC_insn_<mode>"): New insn pattern.
	(UNSPEC_CMPINT): Rename constant to ...
	(UNSPEC_CCU_TO_INT): ... this.
	(UNSPEC_CCZ_TO_INT): New constant.
	(UNSPEC_TDC_INSN): New constant.


I cannot approve the optabs/genopinit/builtin change, although they look
good to me.  We should add documentation of the new named pattern to
doc/md.texi, though.


> +(define_expand "signbit_<mode>"
> +  [(set (reg:CCZ CC_REGNUM)
> +        (unspec:CCZ [(match_operand:BFP 0 "register_operand" "f") 
> +                     (match_dup 2)] 
> +                     UNSPEC_TDC_INSN))
> +   (parallel
> +    [ (set (match_operand:SI 1 "register_operand" "=d")
> +        (unspec:SI [(reg:CCZ CC_REGNUM)] UNSPEC_CCZ_TO_INT))
> +      (clobber (reg:CC CC_REGNUM))])
> +  ]
Common coding style in s390.md is to have closing brackets/parenthesis
at the end of the line, not on a line by themselves.  (Three other
locations with same issue.)

> +  "TARGET_HARD_FLOAT && TARGET_IEEE_FLOAT"
> +  {
> +    operands[2] = GEN_INT (s390_TDC_signbit_set);
> +  }
Coding style is to have the { } for embedded C code sequences start 
in the first column.  Alternatively, for code that is just a single
line, you can use the old "" syntax.
> +)

> +; instruction. The insn's first operand is the register to be tested
Two spaces after '.'.

> +(define_insn "*TDC_insn_<mode>"
> +  [(set (reg:CCZ CC_REGNUM)
> +        (unspec:CCZ [(match_operand:BFP 0 "register_operand" "f") 
> +                     (match_operand:SI 1 "const_int_operand")] UNSPEC_TDC_INSN))
> +  ]
> +  "TARGET_HARD_FLOAT && TARGET_IEEE_FLOAT"
> +  "tc<xde>b\t%0,%1"
> +)
You definitely need the op_type, and possibly the type attribute here,
to allow for correct length computation and scheduling.

> +(define_insn_and_split "*ccz_to_int"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +        (unspec:SI [(match_operand:CCZ 1 "register_operand" "0")]
> +                   UNSPEC_CCZ_TO_INT))
> +   (clobber (reg:CC CC_REGNUM))]
This doesn't actually clobber CC, so you shouldn't have it in
the pattern ...

s390 parts OK with the above changes, provided the middle-end changes
have been approved.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com


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