diff mbox

[SH] Add support for inlined builtin-strcmp (2/2)

Message ID 1382175042.2445.84.camel@yam-132-YW-E178-FTW
State New
Headers show

Commit Message

Oleg Endo Oct. 19, 2013, 9:30 a.m. UTC 
On Fri, 2013-10-18 at 09:59 +0200, Christian Bruel wrote:
> On 10/18/2013 01:05 AM, Oleg Endo wrote:
> > I was wondering, in file sh-mem.c, the new function
> > 'sh4_expand_cmpstr' ... why is it SH4-something?  It's a bit confusing,
> > since cmp/str has been around since ever (i.e. since SH1). Maybe just
> > rename it to 'sh_expand_cmpstr' instead?
> 
> Just historical. (SH4* are our primary SH platforms). The code is
> enabled/tested for all SH1 of course, I will  rename. Thanks .
> 
> >  Maybe just
> > rename it to 'sh_expand_cmpstr' instead?  The function always returns
> > 'true', so maybe just make it return 'void'?
> 
> yes, it's for genericity as I plan to reuse/specialize the code based on
> the count parameter for strncmp to be contributed next.

I already assumed so :)

> >
> > Also, in the expander ...
> >
> > +  [(set (match_operand:SI 0 "register_operand" "")
> > +	(compare:SI (match_operand:BLK 1 "memory_operand" "")
> >
> > ... no need to use empty "" constraints
> 
> OK, thanks

Could you also please remove the quotes around the preparation block:
  "
{
   if (! optimize_insn_for_size_p () && sh4_expand_cmpstr(operands))
      DONE;
   else FAIL;
}")

I've attached two test cases, tested with 
make -k check-gcc RUNTESTFLAGS="sh.exp=strcmp* --target_board=sh-sim
\{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}"

Could you please include them?

Cheers,
Oleg

Comments

Christian Bruel Oct. 20, 2013, 12:24 p.m. UTC | #1 
Hi Oleg,

On 10/19/2013 11:30 AM, Oleg Endo wrote:

>
>
> I've attached two test cases, tested with 
> make -k check-gcc RUNTESTFLAGS="sh.exp=strcmp* --target_board=sh-sim
> \{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}"
>
> Could you please include them?
>
> Cheers,
> Oleg
>
>
thanks for having retested this,  The tests are still not complete for 
RTL generated functions, there are cases where no str/cmp wil be
emitted, because we can predict than the size is less than 4 and so have
a direct fallthru into the byte at a byte loop copying.

Also I will consider a size optimized implementation, so we don't jump
to the library

I will post examples for this shortly (and add them as a testcase) with
a strncmp implementation helper, that pertains to strcmp with constant
strings by the way. Please allow me some time to complete by benchmarking.

thanks  for the hints about removing empty "" is the expanders

Kaz, before proceeding with the next patch, was your approval for 1/2
only or 2/2 with the expander cleanup ?

Many thanks,

Christian
Kaz Kojima Oct. 20, 2013, 10:31 p.m. UTC | #2 
Christian Bruel <christian.bruel@st.com> wrote:
> thanks for having retested this,  The tests are still not complete for 
> RTL generated functions, there are cases where no str/cmp wil be
> emitted, because we can predict than the size is less than 4 and so have
> a direct fallthru into the byte at a byte loop copying.
> 
> Also I will consider a size optimized implementation, so we don't jump
> to the library
> 
> I will post examples for this shortly (and add them as a testcase) with
> a strncmp implementation helper, that pertains to strcmp with constant
> strings by the way. Please allow me some time to complete by benchmarking.
> 
> thanks  for the hints about removing empty "" is the expanders
> 
> Kaz, before proceeding with the next patch, was your approval for 1/2
> only or 2/2 with the expander cleanup ?

Only for 1/2.  The revised 2/2 patch is pre-approved if the usual
tests are Ok, though.  One minor formatting problem:

>+   if (! optimize_insn_for_size_p () && sh4_expand_cmpstr(operands))
>+      DONE;

A space is needed just before (operands).

Regards,
	kaz
diff mbox

Patch

Index: gcc/testsuite/gcc.target/sh/strcmp-2.c
===================================================================
--- gcc/testsuite/gcc.target/sh/strcmp-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/sh/strcmp-2.c	(revision 0)
@@ -0,0 +1,13 @@ 
+/* Check that the __builtin_strcmp function is not inlined when optimizing
+   for size.  */
+/* { dg-do compile { target "sh*-*-*" } } */
+/* { dg-options "-Os" } */
+/* { dg-skip-if "" { "sh*-*-*" } { "-m5*" } { "" } } */
+/* { dg-final { scan-assembler-not "cmp/str" } } */
+/* { dg-final { scan-assembler "jsr|jmp|bsr|bra" } } */
+
+int
+test00 (const char* a, const char* b)
+{
+  return __builtin_strcmp (a, b);
+}
Index: gcc/testsuite/gcc.target/sh/strcmp-1.c
===================================================================
--- gcc/testsuite/gcc.target/sh/strcmp-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/sh/strcmp-1.c	(revision 0)
@@ -0,0 +1,12 @@ 
+/* Check that the __builtin_strcmp function is inlined utilizing cmp/str insn
+   when optimizing for speed.  */
+/* { dg-do compile { target "sh*-*-*" } } */
+/* { dg-options "-O2" } */
+/* { dg-skip-if "" { "sh*-*-*" } { "-m5*" } { "" } } */
+/* { dg-final { scan-assembler "cmp/str" } } */
+
+int
+test00 (const char* a, const char* b)
+{
+  return __builtin_strcmp (a, b);
+}