Re: [Patch] 2/3 MIPS support for builtin __builtin_flush_icache().

[David Daney <ddaney@avtrex.com>]

[-- Attachment #1: Type: text/plain, Size: 7305 bytes --]

Richard Sandiford wrote:
> David Daney <ddaney@avtrex.com> writes:
>   
>> @@ -4171,6 +4173,72 @@ (define_insn "cprestore"
>>  }
>>    [(set_attr "type" "store")
>>     (set_attr "length" "4,12")])
>> +
>> +(define_expand "flush_icache"
>> +  [(match_operand:SI 0 "general_operand" "r")
>> +   (match_operand:SI 1 "general_operand" "r")]
>>     
>
> No constraints in a define_expand; just remove the "r" strings altogether.
>
>   
Fixed.

>> +  if (ISA_HAS_SYNCI)
>> +    {
>> +      emit_insn (gen_synci_loop (copy_rtx (operands[0]),
>> +                                           copy_rtx (operands[1])));
>> +      emit_insn (gen_clear_hazard ());
>>     
>
> Odd indentation.  No need to call copy_rtx here; the operands are only
> used this once.
>
>   
Right.  I guess I am a little unclear on when copy_rtx is required.

>> +    /* Flush both caches.  We need to flush the data cache in case
>> +       the system has a write-back cache.  */
>> +    /* ??? Should check the return value for errors.  */
>> +    if (mips_cache_flush_func && mips_cache_flush_func[0])
>> +      emit_library_call (gen_rtx_SYMBOL_REF (Pmode, mips_cache_flush_func),
>> +                         0, VOIDmode, 3, copy_rtx (operands[0]), Pmode,
>> +                         copy_rtx (operands[1]), TYPE_MODE (integer_type_node),
>> +                         GEN_INT (3), TYPE_MODE (integer_type_node));
>> +  DONE;
>>     
>
> Same copy_rtx comment here.  TBH, I'm not sure what the "???" comment
> refers to.
>
>   
I just pased this part in from its old home in mips.h.  The ??? comment 
came with it.  It is gone now.

>> +(define_insn "synci_loop"
>> +  [(unspec_volatile[(match_operand:SI 0 "general_operand" "r")
>> +                    (match_operand:SI 1 "general_operand" "r")
>> +                    (clobber (match_scratch:SI 2 "=0"))
>> +                    (clobber (match_scratch:SI 3 "=1"))
>> +                    (clobber (match_scratch:SI 4 "=r"))
>> +		    (clobber (match_scratch:SI 5 "=r"))]
>> +                    UNSPEC_SYNCI_LOOP)]
>>     
>
> Is there any particular need to force operand 2 to be the same as
> operand 0, and operand 3 to be the same as operand 1?  Since these
> clobbers aren't earlyclobbers, I would have expected the register
> allocators to be able to reuse operands 0 and 1 as scratch registers
> if it thought that doing so was useful.  _Forcing_ it to reuse them
> seems unnecessarily restrictive.
>
> Minor nit, but please use "d" rather than "r", for consistency with
> other mips.md patterns.  I realise this pattern will never be used for
> MIPS16, but even so.
>   
The first one was to force the register to be reused so I didn't have to 
waste an instruction making a copy of it.  The second was gratuitous.  
It is moot though as this part is gone now.


> I wonder if would be better
> to simply expand this as rtl, with special patterns for the "rdhwr",
> "synci" and "sync" instructions.  This is similar to what we do for
> other largish patterns.  That's certainly not a requirement though;
> I'm not even sure it makes sense.  It's just an idea-cum-question.
>
>   
That is what I did.


>> +(define_insn "clear_hazard"
>> +  [(unspec_volatile [(clobber (match_scratch:SI 0 "=r"))] UNSPEC_CLEAR_HAZARD)
>> +   (clobber (reg:SI 31))]
>>     
>
> Clobbers aren't expected inside unspec_volatiles.  I think this should be:
>
>   [(unspec_volatile [(const_int 0)] UNSPEC_CLEAR_HAZARD)
>    (clobber (match_scratch:SI 0 "=d"))
>    (clobber (reg:SI 31))]
>
> (where "(const_int 0)" is the traditional "I don't take any arguments" hack)
>
>   
Fixed.

>> +  "ISA_HAS_SYNCI"
>> +{
>> +  return ".set\tpush\n"
>> +         "\t.set\tnoreorder\n"
>> +         "\t.set\tnomacro\n"
>> +         "\tbal\t1f\n"
>> +         "\tnop\n"
>> +         "1:\taddiu\t%0,$31,12\n"
>> +         "\tjr.hb\t%0\n"
>> +         "\tnop\n"
>> +         "\t.set\tpop";
>> +}
>>     
>
> Following on from your comment about delay slots, it also strikes me
> that the "bal; nop; addiu" sequence is almost always longer than the
> sequence to load a local address into a register.  I think the only
> exception is static n64 with -mno-sym32.  I wonder if it would be
> worth using a sequence like:
>
>     rtx label, target, insn;
>
>     label = gen_label_rtx ();
>     target = force_reg (Pmode, gen_rtx_LABEL_REF (Pmode, label));
>     insn = emit_jump_insn (gen_indirect_jump_hb (target));
>     REG_NOTES (insn) = gen_rtx_INSN_LIST (REG_LABEL, label, REG_NOTES (insn));
>     emit_label (label);
>
> where indirect_jump_hb would just be something like:
>
> (define_insn "indirect_jump_hb"
>   [(set (pc) (match_operand "pmode_register_operand" "d"))
>    (unspec_volatile [(const_int 0)] UNSPEC_JR_HB)]
>   "ISA_HAS_SYNCI"
>   "jr.hb\t%0"
>   [(set_attr "type" "jump")])
>
> (all untested).  I think we might then be able to fill the delay slot
> from the target of the jump, which should be OK in this context.
> Again, this is just an idea-cum-question.
>
>   
I spent way too much time trying to get that to work.  For now I left it 
mostly unchanged from the original patch.

The problem is that the CFG things just don't seem to be able to handle:
1) Jump insns where the body is a (parallel ...) instead of a (set (pc) 
...).
2) Unconditional jumps where the target and fall through edges goto the 
same place.

I kept the call to mips_cache_flush_func pretty much unchanged because I 
didn't want to break compatibility with the command line options that 
allow this to be changed.   Also I added CLEAR_INSN_CACHE that expands 
in libgcc that is used if  __builtin___clear_cache() cannot be expanded 
in-line.  This is slightly inefficient as it adds an extra function call 
layer through libgcc and has to do the arithmetic to convert the 
function parameters, but I am thinking I shouldn't be concerned about a 
few extra cycles doing this when we are about to do a system call.  The 
alternative is to have __builtin___clear_cache() directly expand to 
mips_cache_flush_func and leave libgcc out of the picture for MIPS. 

Attached is the new version of the patch.

Currently bootstrapping/testing on x86_64-unknown-linux-gnu, 
mipsel-unknown-linux-gnu (--with-arch=[mips32|mips32r2]).

OK to commit if the target independent portion is approved?

2007-07-04  David Daney  <ddaney@avtrex.com>

    * config/mips/mips.h (mips_builtin_clear_cache_inline_p): Declare
    function.
    (TARGET_BUILTIN_CLEAR_CACHE_INLINE_P): Define target hook.
    (ISA_HAS_SYNCI): New target capability predicate.
    (CLEAR_INSN_CACHE): Define libgcc2 hook.
    (INITIALIZE_TRAMPOLINE): Emit flush_icache instead library call.
    * config/mips/netbsd.h (CLEAR_INSN_CACHE): Define libgcc2 hook.
    * config/mips/mips-protos.h (mips_expand_synci_loop): Declare
    function.
    * config/mips/mips.md (UNSPEC_CLEAR_HAZARD): New constant.
    (UNSPEC_RDHWR): Same.
    (UNSPEC_SYNCI): Same.
    (UNSPEC_SYNC): Same.
    (flush_icache): New expand.
    (clear_cache): Same.
    (sync): New insn.
    (synci): Same.
    (rdhwr): Same.
    (clear_hazard): Same.
    * config/mips/mips.c (mips_builtin_clear_cache_inline_p): New function.
    (mips_expand_synci_loop): Same.
    * testsuite/gcc.target/mips/clear-cache-1.c: New test.
    * testsuite/gcc.target/mips/clear-cache-2.c: New test.





[-- Attachment #2: flush-cache2.diff --]
[-- Type: text/x-patch, Size: 9970 bytes --]

Index: config/mips/mips.h
===================================================================
--- config/mips/mips.h	(revision 125997)
+++ config/mips/mips.h	(working copy)
@@ -136,6 +136,10 @@ extern const struct mips_cpu_info mips_c
 extern const struct mips_cpu_info *mips_arch_info;
 extern const struct mips_cpu_info *mips_tune_info;
 extern const struct mips_rtx_cost_data *mips_cost;
+
+extern bool mips_builtin_clear_cache_inline_p (void);
+#undef TARGET_BUILTIN_CLEAR_CACHE_INLINE_P
+#define TARGET_BUILTIN_CLEAR_CACHE_INLINE_P mips_builtin_clear_cache_inline_p
 #endif
 
 /* Macros to silence warnings about numbers being signed in traditional
@@ -770,6 +774,10 @@ extern const struct mips_rtx_cost_data *
 				 || ISA_MIPS32R2			\
 				 || ISA_MIPS64				\
 				 || TARGET_MIPS5500)
+
+/* ISA includes synci, jr.hb and jalr.hb */
+#define ISA_HAS_SYNCI ISA_MIPS32R2
+
 \f
 /* Add -G xx support.  */
 
@@ -2108,6 +2116,17 @@ typedef struct mips_args {
 #define CACHE_FLUSH_FUNC "_flush_cache"
 #endif
 
+/* Clear the instruction cache from `beg' to `end'.  */
+#undef CLEAR_INSN_CACHE
+#define CLEAR_INSN_CACHE(BEG, END)				\
+{								\
+  extern void _flush_cache (char *b, int l, int f);		\
+  if (__builtin_clear_cache_inline_p())				\
+    __builtin___clear_cache ((BEG), (END));			\
+  else								\
+    _flush_cache ((BEG), ((char *)(END) - (char *)(BEG)), 3);	\
+}
+
 /* A C statement to initialize the variable parts of a trampoline.
    ADDR is an RTX for the address of the trampoline; FNADDR is an
    RTX for the address of the nested function; STATIC_CHAIN is an
@@ -2122,15 +2141,7 @@ typedef struct mips_args {
   chain_addr = plus_constant (func_addr, GET_MODE_SIZE (ptr_mode));	    \
   emit_move_insn (gen_rtx_MEM (ptr_mode, func_addr), FUNC);		    \
   emit_move_insn (gen_rtx_MEM (ptr_mode, chain_addr), CHAIN);		    \
-									    \
-  /* Flush both caches.  We need to flush the data cache in case	    \
-     the system has a write-back cache.  */				    \
-  /* ??? Should check the return value for errors.  */			    \
-  if (mips_cache_flush_func && mips_cache_flush_func[0])		    \
-    emit_library_call (gen_rtx_SYMBOL_REF (Pmode, mips_cache_flush_func),   \
-		       0, VOIDmode, 3, ADDR, Pmode,			    \
-		       GEN_INT (TRAMPOLINE_SIZE), TYPE_MODE (integer_type_node),\
-		       GEN_INT (3), TYPE_MODE (integer_type_node));	    \
+  emit_insn (gen_flush_icache (copy_rtx (ADDR), GEN_INT (TRAMPOLINE_SIZE))); \
 }
 \f
 /* Addressing modes, and classification of registers for them.  */
Index: config/mips/netbsd.h
===================================================================
--- config/mips/netbsd.h	(revision 125997)
+++ config/mips/netbsd.h	(working copy)
@@ -190,6 +190,16 @@ Boston, MA 02110-1301, USA.  */
 #undef CACHE_FLUSH_FUNC
 #define CACHE_FLUSH_FUNC "_cacheflush"
 
+/* Clear the instruction cache from `beg' to `end'.  */
+#undef CLEAR_INSN_CACHE
+#define CLEAR_INSN_CACHE(BEG, END)				\
+{								\
+  extern void _cacheflush (char *b, int l, int f);		\
+  if (__builtin_clear_cache_inline_p())				\
+    __builtin___clear_cache ((BEG), (END));			\
+  else								\
+    _cacheflush ((BEG), ((char *)(END) - (char *)(BEG)), 3);	\
+}
 
 /* Make gcc agree with <machine/ansi.h> */
 
Index: config/mips/mips-protos.h
===================================================================
--- config/mips/mips-protos.h	(revision 125997)
+++ config/mips/mips-protos.h	(working copy)
@@ -1,6 +1,6 @@
 /* Prototypes of target machine for GNU compiler.  MIPS version.
    Copyright (C) 1989, 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2001, 2002, 2003, 2004, 2005 Free Software Foundation, Inc.
+   1999, 2001, 2002, 2003, 2004, 2005, 2007 Free Software Foundation, Inc.
    Contributed by A. Lichnewsky (lich@inria.inria.fr).
    Changed by Michael Meissner	(meissner@osf.org).
    64-bit r4000 support by Ian Lance Taylor (ian@cygnus.com) and
@@ -185,6 +185,7 @@ extern void mips_expand_call (rtx, rtx, 
 extern void mips_emit_fcc_reload (rtx, rtx, rtx);
 extern void mips_set_return_address (rtx, rtx);
 extern bool mips_expand_block_move (rtx, rtx, rtx);
+extern void mips_expand_synci_loop (rtx, rtx);
 
 extern void init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx);
 extern void function_arg_advance (CUMULATIVE_ARGS *, enum machine_mode,
Index: config/mips/mips.md
===================================================================
--- config/mips/mips.md	(revision 125997)
+++ config/mips/mips.md	(working copy)
@@ -50,6 +50,10 @@ (define_constants
    (UNSPEC_TLS_GET_TP		28)
    (UNSPEC_MFHC1		31)
    (UNSPEC_MTHC1		32)
+   (UNSPEC_CLEAR_HAZARD		33)
+   (UNSPEC_RDHWR		34)
+   (UNSPEC_SYNCI		35)
+   (UNSPEC_SYNC			36)
 
    (UNSPEC_ADDRESS_FIRST	100)
 
@@ -4171,6 +4175,82 @@ (define_insn "cprestore"
 }
   [(set_attr "type" "store")
    (set_attr "length" "4,12")])
+
+;; Flush the instruction cache starting as operands[0] with length
+;; operands[1].
+(define_expand "flush_icache"
+  [(match_operand:SI 0 "pmode_register_operand")
+   (match_operand:SI 1 "register_operand")]
+  ""
+  "
+{
+  if (ISA_HAS_SYNCI)
+    {
+      rtx end_addr = gen_reg_rtx (Pmode);
+      emit_insn (gen_rtx_SET (VOIDmode, end_addr,
+      			      gen_rtx_PLUS (Pmode, operands[0], operands[1])));
+      emit_insn (gen_clear_cache (operands[0], end_addr));
+    }
+  else
+    /* Flush both caches.  We need to flush the data cache in case
+       the system has a write-back cache.  */
+    if (mips_cache_flush_func && mips_cache_flush_func[0])
+      emit_library_call (gen_rtx_SYMBOL_REF (Pmode, mips_cache_flush_func),
+                         0, VOIDmode, 3, operands[0], Pmode,
+                         operands[1], TYPE_MODE (integer_type_node),
+                         GEN_INT (3), TYPE_MODE (integer_type_node));
+  DONE;
+}")
+
+;; Expand in-line code to clear the instruction cache between operand[0] and
+;; operand[1].
+(define_expand "clear_cache"
+  [(match_operand:SI 0 "general_operand")
+   (match_operand:SI 1 "general_operand")]
+  "ISA_HAS_SYNCI"
+  "
+{
+  mips_expand_synci_loop (operands[0], operands[1]);
+  emit_insn (gen_clear_hazard ());
+  DONE;
+}")
+
+(define_insn "sync"
+  [(unspec_volatile [(const_int 0)] UNSPEC_SYNC)]
+  ""
+  "sync"
+  [(set_attr "length" "4")])
+
+(define_insn "synci"
+  [(unspec_volatile [(match_operand:SI 0 "general_operand" "d")] UNSPEC_SYNCI)]
+  ""
+  "synci\t0(%0)"
+  [(set_attr "length" "4")])
+
+(define_insn "rdhwr"
+  [(set (match_operand:SI 0 "general_operand" "=d")
+        (unspec_volatile [(match_operand:SI 1 "immediate_operand" "n")]
+        UNSPEC_RDHWR))]
+  ""
+  "rdhwr\t%0,$%1"
+  [(set_attr "length" "4")])
+
+(define_insn "clear_hazard"
+  [(unspec_volatile [(const_int 0)] UNSPEC_CLEAR_HAZARD)
+   (clobber (reg:SI 31))]
+  "ISA_HAS_SYNCI"
+{
+  return ".set\tpush\n"
+         "\t.set\tnoreorder\n"
+         "\t.set\tnomacro\n"
+         "\tbal\t1f\n"
+         "\tnop\n"
+         "1:\taddiu\t$31,$31,12\n"
+         "\tjr.hb\t$31\n"
+         "\tnop\n"
+         "\t.set\tpop";
+}
+  [(set_attr "length" "20")])
 \f
 ;; Block moves, see mips.c for more details.
 ;; Argument 0 is the destination
Index: config/mips/mips.c
===================================================================
--- config/mips/mips.c	(revision 125997)
+++ config/mips/mips.c	(working copy)
@@ -3759,6 +3759,47 @@ mips_block_move_loop (rtx dest, rtx src,
     mips_block_move_straight (dest, src, leftover);
 }
 \f
+
+/* Return true if we will expand __builtin_clear_cache() in-line. */
+bool
+mips_builtin_clear_cache_inline_p (void)
+{
+  return ISA_HAS_SYNCI;
+}
+
+/* Expand a loop of synci insns */
+void
+mips_expand_synci_loop (rtx begin, rtx end)
+{
+  rtx end_addr, inc, label, label_ref, cmp, cmp_result;
+
+  begin = force_reg(Pmode, begin);
+  end = force_reg(Pmode, end);
+
+  /* Load INC with the cache line size (rdhwr INC,$1). */
+  inc = gen_reg_rtx (SImode);
+  emit_insn (gen_rdhwr (inc ,gen_rtx_CONST_INT(SImode, 1)));
+
+  /* Loop back to here. */
+  label = gen_label_rtx ();
+  emit_label (label);
+
+  emit_insn (gen_synci (begin));
+
+  cmp = gen_reg_rtx (SImode);
+  emit_insn (gen_rtx_SET (VOIDmode, cmp, gen_rtx_GTU (Pmode, begin, end)));
+
+  emit_insn (gen_rtx_SET (VOIDmode, begin, gen_rtx_PLUS (Pmode, begin, inc)));
+
+  label_ref = gen_rtx_LABEL_REF (VOIDmode, label);
+  cmp_result = gen_rtx_EQ (VOIDmode, cmp, gen_rtx_CONST_INT(SImode, 0));
+  emit_jump_insn (gen_rtx_SET (VOIDmode, pc_rtx,
+                               gen_rtx_IF_THEN_ELSE (VOIDmode, cmp_result,
+                                                     label_ref,
+                                                     pc_rtx)));
+  emit_insn (gen_sync ());
+}
+\f
 /* Expand a movmemsi instruction.  */
 
 bool
Index: testsuite/gcc.target/mips/clear-cache-1.c
===================================================================
--- testsuite/gcc.target/mips/clear-cache-1.c	(revision 0)
+++ testsuite/gcc.target/mips/clear-cache-1.c	(revision 0)
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -mips32r2" } */
+/* { dg-final { scan-assembler "synci" } } */
+/* { dg-final { scan-assembler "jr.hb" } } */
+/* { dg-final { scan-assembler-not "__clear_cache" } } */
+
+void f()
+{
+  int size = 40;
+  char *memory = __builtin_alloca(size);
+  __builtin___clear_cache(memory, memory + size);
+}
+
Index: testsuite/gcc.target/mips/clear-cache-2.c
===================================================================
--- testsuite/gcc.target/mips/clear-cache-2.c	(revision 0)
+++ testsuite/gcc.target/mips/clear-cache-2.c	(revision 0)
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -mips32" } */
+/* { dg-final { scan-assembler-not "synci" } } */
+/* { dg-final { scan-assembler-not "jr.hb" } } */
+/* { dg-final { scan-assembler "__clear_cache" } } */
+
+void f()
+{
+  int size = 40;
+  char *memory = __builtin_alloca(size);
+  __builtin___clear_cache(memory, memory + size);
+}
+