Richard Sandiford wrote: > David Daney 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 * 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.