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,SH] Add SH2A new instructions 6/6


"Naveen H.S." <naveen.hs@kpitcummins.com> wrote:
> The implemented bit instructions resulted in very good optimization
> of the bit operations.

Could you show us some numbers how this results in good
optimizations?

> However, the bit instructions are not getting
> generated more frequently after the above mentioned patch. Please
> suggest the modifications to be done on the present patch to generate
> bit instructions as earlier.

I have no idea.  In general, if you don't get the expected
instruction, the rtl and tree debug dumps would be your friends.

I'm concerned with that the use of bitwise memory instructions
impacts on the performance and the code size.
I guess that it will make many 8-bit memory accesses for
operations on bit fields which are currently done on 32-bit
registers at a time.
Although those bitwise memory insns would be better for
the volatile access of a bit in memory like as your testcases
added for new insns or other rather special cases, it might
be worse on the average working set of programs.

Please consider the comment in the previous mail:
> You should confirm that the use of SH2A bitwise operations doesn't
> cause performance/size regressions with real applications.  If it's
> hard to prove, the use of bitwise operations should be controlled
> with the option -mbitops or some such which is off by default.

Here are some comments on each parts of the patch.

There are no tests for "b*not" instructions.  We should avoid
adding things which we can't test at all.  If they are hard
to use in the current compiler, drop the related hunks from
the patch:

> --- gcc-4.3-20070921/gcc/config/sh/predicates.md	2007-08-02 16:19:31.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/config/sh/predicates.md	2007-10-24 12:51:53.000000000 +0530
> @@ -516,6 +516,11 @@
>  (define_predicate "logical_operator"
>    (match_code "and,ior,xor"))
>  
> +;; Recognize valid operators for bitnot instructions.
> +
> +(define_predicate "andor_operator"
> +  (match_code "and,ior"))
> +
>  ;; Like arith_reg_operand, but for register source operands of narrow
>  ;; logical SHMEDIA operations: forbid subregs of DImode / TImode regs.

and bldnot_m2a, bandnot_m2a, bornot_m2a, bitnot_m2a insns.

bld_genreg, *bld_genregqi, bit_m2a and bitnot_m2a generate
multiple instructions as text and also have no length attribute.
These parts are not Ok and should be dropped from the patch.

> --- gcc-4.3-20070921/gcc/config/sh/sh.c	2007-10-24 12:21:48.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/config/sh/sh.c	2007-10-24 12:26:33.000000000 +0530
> @@ -678,6 +678,9 @@ print_operand_address (FILE *stream, rtx
>     'U'  Likewise for {LD,ST}{HI,LO}.
>     'V'  print the position of a single bit set.
>     'W'  print the position of a single bit cleared.
> +   'f'  output the bit operator.
> +   'e'  output the bitnot operator.
[snip]
> +
> +    case 'f':
> +      switch (GET_CODE (x))
> +        {
> +        case IOR:
> +          fprintf (stream, "or");
> +          break;
> +        case XOR:
> +          fprintf (stream, "xor");
> +          break;
> +        case AND:
> +          fprintf (stream, "and");
> +          break;
> +        default:
> +          break;
> +        }
> +      break;
> +
> +    case 'e':
> +      switch (GET_CODE (x))
> +        {
> +        case IOR:
> +          fprintf (stream, "ornot");
> +          break;
> +        case AND:
> +          fprintf (stream, "andnot");
> +          break;
> +        default:
> +          break;
> +        }
> +      break;

We should avoid adding new operand formats if possible.  Even
when we add all b*not insns in the future, we can use the explicit
names in the asm text.

> --- gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-band.c	1970-01-01 05:30:00.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-band.c	2007-10-24 12:28:42.000000000 +0530
> @@ -0,0 +1,94 @@
> +/* Testcase to check generation of a SH2A specific instruction for
> +   "BAND.B #imm3, @(disp12, Rn)" 8-bit  */

The last "8-bit" is pointless.
An English sentence needs a period at the end.  Other new tests
have the same issue.

> +/* { dg-skip-if "" { "sh*-*-*" } "*" "-m2a -m2a-nofpu -m2a-single
> +   -m2a-single-only" }  */

I thought that a dg- directive in testcase should be a single line,
though I'm wrong about it.  It'd be safer to make it to a one long
line.

> --- gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-bldbst.c	1970-01-01 05:30:00.000000000 +0530
> +++ tars/gcc-4.3-20070921/gcc/testsuite/gcc.target/sh/sh2a-bldbst.c	2007-10-24 12:33:29.000000000 +0530
> @@ -0,0 +1,47 @@
> +/* Testcase to check generation of a SH2A specific instruction for 8-bit
> +   union -
> + *   BLD #imm3, (Rn)
> + *   BLD.B #imm3, @(disp12, Rn)
> + *   BST.B #imm3, @(disp12, Rn)
> +*/

"instruction for 8-bit union" is bogus.  Maybe

/* A testcase to check generation of the following SH2A specific
   instructions.

    BLD #imm3, Rn
    BLD.B #imm3, @(disp12, Rn)
    BST.B #imm3, @(disp12, Rn)
 */

Please use correct formats of ChangeLog entry.  See existing
ChangeLog entries as your guide.  New testcases need new entries
in gcc/testsuite/ChangeLog.
Some examples about format issues here.  I've given alternatives
in a few cases to clarify the format problems.

> 2007-10-25	Naveen.H.S naveen.hs@kpitcummins.com

  200z-xx-yy  Naveen.H.S  <naveen.hs@kpitcummins.com>
            ^^ 2 spaces ^^^ 2 spaces and use <email-address>

> 	* config/sh/constraints.md: K03, K12, Sbw : New constraints.

	* config/sh/constraints.md (K03, K12, Sbw): New constraints.

> 	* config/sh/predicates.md (bitwise_memory_operand) : New
> predicate.

Indentation or a long line.  Remove a space after ).

> 	* config/sh/sh.c (print_operand): "f" bit operator
> 	"e" bitnot operator.
> 	"t" prints memory address of a register.

Use English sentences.  Perhaps simply

 	* config/sh/sh.c (print_operand): Add %t operand code.

> 	* config/sh/sh.h (sh_args): Add condition for SH2A.

Perhaps

	* config/sh/sh.h (GO_IF_LEGITIMATE_INDEX): Add condition
	for SH2A.
?

> 	* config/sh/sh.md (extendqisi2_compact): enable 4-byte mov.b 
> 	instructions.
> 	(extendqihi2): enable 4-byte mov.b instructions.
> 	(movqi_i): enable 4-byte mov.b instructions.

English sentences in ChangeLog should start with a capital.

	* config/sh/sh.md (extendqisi2_compact): Add the alternative
	for SH2A 4-byte mov.b.
	(extendqihi2): Likewise.
	(movqi_i): Likewise.

> 	(insv): Add conditions to generate BSET, BCLR and BST
> instructions in 
> 	memory mode.
> 	(extv): Add conditions to generate BLD instruction in memory
> mode.
> 	(extzv): Add conditions to generate BLD instruction in memory
> mode.

Long lines?  It looks that capitalized opcodes are not appropriate here.

	(insv): Use bset, bclr and bst instructions for SH2A if possible.
	(extv): Use bld instruction for SH2A if possible.
	(extzv): Likewise.

> 	bclr_m2a, bset_m2a, bst_m2a, bld_m2a, bld_reg, *bld_regqi,
> bld_reg1, 
> 	bld_regqi1, bldnot_m2a, band_m2a, bandnot_m2a, bor_m2a,

Parentheses needed.  It looks that there are no bld_reg1 and
bld_regqi1 in your patch.

	(bclr_m2a, bset_m2a, bst_m2a, bld_m2a, bld_reg, *bld_regqi,
	band_m2a, bor_m2a, bxor_m2a): New insns.

Regards,
	kaz


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