This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] add insn implementing signbit to middle end and s390
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: gellerich at de dot ibm dot com (Wolfgang Gellerich)
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Fri, 30 Mar 2007 16:21:41 +0200 (CEST)
- Subject: 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