> -----Original Message----- > From: Dragan Mladjenovic > Sent: 17 August 2021 17:59 > To: 'Andrew Pinski' <pins...@gmail.com> > Cc: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org> > Subject: RE: [PATCH] [MIPS] Hazard barrier return support > > > > > -----Original Message----- > > From: Dragan Mladjenovic > > Sent: 16 August 2021 22:40 > > To: 'Andrew Pinski' <pins...@gmail.com> > > Cc: gcc-patches@gcc.gnu.org > > Subject: RE: [PATCH] [MIPS] Hazard barrier return support > > > > > > > > > -----Original Message----- > > > From: Andrew Pinski [mailto:pins...@gmail.com] > > > Sent: 16 August 2021 21:17 > > > To: Dragan Mladjenovic <ot_dragan.mladjeno...@mediatek.com> > > > Cc: gcc-patches@gcc.gnu.org > > > Subject: Re: [PATCH] [MIPS] Hazard barrier return support > > > > > > On Mon, Aug 16, 2021 at 7:43 AM Dragan Mladjenovic via Gcc-patches > > > <gcc- patc...@gcc.gnu.org> wrote: > > > > > > > > This patch allows a function to request clearing of all > > > > instruction and execution hazards upon normal return via > > > > __attribute__ > > > ((use_hazard_barrier_return)). > > > > > > > > 2017-04-25 Prachi Godbole <prachi.godb...@imgtec.com> > > > > > > > > gcc/ > > > > * config/mips/mips.h (machine_function): New variable > > > > use_hazard_barrier_return_p. > > > > * config/mips/mips.md (UNSPEC_JRHB): New unspec. > > > > (mips_hb_return_internal): New insn pattern. > > > > * config/mips/mips.c (mips_attribute_table): Add attribute > > > > use_hazard_barrier_return. > > > > (mips_use_hazard_barrier_return_p): New static function. > > > > (mips_function_attr_inlinable_p): Likewise. > > > > (mips_compute_frame_info): Set use_hazard_barrier_return_p. > > > > Emit error for unsupported architecture choice. > > > > (mips_function_ok_for_sibcall, mips_can_use_return_insn): > > > > Return false for use_hazard_barrier_return. > > > > (mips_expand_epilogue): Emit hazard barrier return. > > > > * doc/extend.texi: Document use_hazard_barrier_return. > > > > > > > > gcc/testsuite/ > > > > * gcc.target/mips/hazard-barrier-return-attribute.c: New test. > > > > --- > > > > Rehash of original patch posted by Prachi with minimal changes. > > > > Tested against mips-mti-elf with mips32r2/-EB and mips32r2/-EB/- > > micromips. > > > > > > > > gcc/config/mips/mips.c | 58 +++++++++++++++++-- > > > > gcc/config/mips/mips.h | 3 + > > > > gcc/config/mips/mips.md | 15 +++++ > > > > gcc/doc/extend.texi | 6 ++ > > > > .../mips/hazard-barrier-return-attribute.c | 20 +++++++ > > > > 5 files changed, 98 insertions(+), 4 deletions(-) create mode > > > > 100644 > > > > gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute.c > > > > > > > > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index > > > > 89d1be6cea6..6ce12fce52e 100644 > > > > --- a/gcc/config/mips/mips.c > > > > +++ b/gcc/config/mips/mips.c > > > > @@ -630,6 +630,7 @@ static const struct attribute_spec > > > mips_attribute_table[] = { > > > > mips_handle_use_shadow_register_set_attr, NULL }, > > > > { "keep_interrupts_masked", 0, 0, false, true, true, false, NULL, > NULL }, > > > > { "use_debug_exception_return", 0, 0, false, true, true, false, > > > > NULL, NULL }, > > > > + { "use_hazard_barrier_return", 0, 0, true, false, false, false, > > > > + NULL, NULL }, > > > > { NULL, 0, 0, false, false, false, false, NULL, NULL } > > > > }; > > > > > > > > @@ -1309,6 +1310,16 @@ mips_use_debug_exception_return_p (tree > > > type) > > > > TYPE_ATTRIBUTES (type)) != NULL; } > > > > > > > > +/* Check if the attribute to use hazard barrier return is set for > > > > + the function declaration DECL. */ > > > > + > > > > +static bool > > > > +mips_use_hazard_barrier_return_p (const_tree decl) { > > > > + return lookup_attribute ("use_hazard_barrier_return", > > > > + DECL_ATTRIBUTES (decl)) != NULL; } > > > > + > > > > /* Return the set of compression modes that are explicitly required > > > > by the attributes in ATTRIBUTES. */ > > > > > > > > @@ -1494,6 +1505,19 @@ mips_can_inline_p (tree caller, tree callee) > > > > return default_target_can_inline_p (caller, callee); } > > > > > > > > +/* Implement TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P. > > > > + > > > > + A function requesting clearing of all instruction and execution > hazards > > > > + before returning cannot be inlined - thereby not clearing any > > > > hazards. > > > > + All our other function attributes are related to how out-of-line > > > > copies > > > > + should be compiled or called. They don't in themselves > > > > + prevent inlining. */ > > > > + > > > > +static bool > > > > +mips_function_attr_inlinable_p (const_tree decl) { > > > > + return !mips_use_hazard_barrier_return_p (decl); } > > > > + > > > > /* Handle an "interrupt" attribute with an optional argument. */ > > > > > > > > static tree > > > > @@ -7921,6 +7945,11 @@ mips_function_ok_for_sibcall (tree decl, > > > > tree > > > exp ATTRIBUTE_UNUSED) > > > > && !targetm.binds_local_p (decl)) > > > > return false; > > > > > > > > + /* Can't generate sibling calls if returning from current function > > > > using > > > > + hazard barrier return. */ > > > > + if (mips_use_hazard_barrier_return_p (current_function_decl)) > > > > + return false; > > > > + > > > > /* Otherwise OK. */ > > > > return true; > > > > } > > > > @@ -11008,6 +11037,17 @@ mips_compute_frame_info (void) > > > > } > > > > } > > > > > > > > + /* Determine whether to use hazard barrier return or not. */ > > > > + if (mips_use_hazard_barrier_return_p (current_function_decl)) > > > > + { > > > > + if (mips_isa_rev < 2) > > > > + error ("hazard barrier returns require a MIPS32r2 > > > > + processor or greater"); > > > > > > Just a small nit, is MIPS64r2 ok too? Also did you did you test it > > > for MIPS64 too? I still partly care about MIPS64. > > I'll respin the tests for mips64r2 and mips64 just in case. > > > > This check would fail for -mips64. GAS accepts jr.hb for both '.set > > mips64' and '.set mips64r2' and '.set mips32' for that matter. > > mips-opc.c has the following > > comment: > > /* jr.hb is officially MIPS{32,64}R2, but it works on R1 as jr with > > the same hazard barrier effect. */ > > > > Regards, > > Dragan > > > > FYI the change introduces no new regression for mips-mti-elf > mips64r2/mabi=64/EB. > > Best regards, > > Dragan > > > > > > > Thanks, > > > Andrew > > > > > > > + else if (TARGET_MIPS16) > > > > + error ("hazard barrier returns are not supported for > > > > + MIPS16 > > > functions"); > > > > + else > > > > + cfun->machine->use_hazard_barrier_return_p = true; > > > > + } > > > > + > > > > frame = &cfun->machine->frame; > > > > memset (frame, 0, sizeof (*frame)); > > > > size = get_frame_size (); > > > > @@ -12671,7 +12711,8 @@ mips_expand_epilogue (bool sibcall_p) > > > > && !crtl->calls_eh_return > > > > && !sibcall_p > > > > && step2 > 0 > > > > - && mips_unsigned_immediate_p (step2, 5, 2)) > > > > + && mips_unsigned_immediate_p (step2, 5, 2) > > > > + && !cfun->machine->use_hazard_barrier_return_p) > > > > use_jraddiusp_p = true; > > > > else > > > > /* Deallocate the final bit of the frame. */ @@ -12712,6 > > > > +12753,11 @@ mips_expand_epilogue (bool sibcall_p) > > > > else > > > > emit_jump_insn (gen_mips_eret ()); > > > > } > > > > + else if (cfun->machine->use_hazard_barrier_return_p) > > > > + { > > > > + rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM); > > > > + emit_jump_insn (gen_mips_hb_return_internal (reg)); > > > > + } > > > > else > > > > { > > > > rtx pat; > > > > @@ -12770,6 +12816,11 @@ mips_can_use_return_insn (void) > > > > if (cfun->machine->interrupt_handler_p) > > > > return false; > > > > > > > > + /* Even if the function has a null epilogue, generating hazard > > > > + barrier > > > return > > > > + in epilogue handler is a lot cleaner and more manageable. > > > > + */ if (cfun->machine->use_hazard_barrier_return_p) > > > > + return false; > > > > + > > > > if (!reload_completed) > > > > return false; > > > > > > > > @@ -22777,10 +22828,9 @@ mips_asm_file_end (void) > > > > > > > > #undef TARGET_ATTRIBUTE_TABLE > > > > #define TARGET_ATTRIBUTE_TABLE mips_attribute_table > > > > -/* All our function attributes are related to how out-of-line > > > > copies > > should > > > > - be compiled or called. They don't in themselves prevent inlining. > > > > */ > > > > + > > > > #undef TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P > > > > -#define TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P > > > > hook_bool_const_tree_true > > > > +#define TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P > > > > +mips_function_attr_inlinable_p > > > > > > > > #undef TARGET_EXTRA_LIVE_ON_ENTRY #define > > > > TARGET_EXTRA_LIVE_ON_ENTRY mips_extra_live_on_entry diff --git > > > > a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index > > > > 47aac9d3d61..bae9ffe9b3c 100644 > > > > --- a/gcc/config/mips/mips.h > > > > +++ b/gcc/config/mips/mips.h > > > > @@ -3376,6 +3376,9 @@ struct GTY(()) machine_function { > > > > > > > > /* True if GCC stored callee saved registers in the frame header. */ > > > > bool use_frame_header_for_callee_saved_regs; > > > > + > > > > + /* True if the function should generate hazard barrier return. > > > > + */ bool use_hazard_barrier_return_p; > > > > }; > > > > #endif > > > > > > > > diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md > > > > index > > > > 455b9b802f6..dee71dc1fb0 100644 > > > > --- a/gcc/config/mips/mips.md > > > > +++ b/gcc/config/mips/mips.md > > > > @@ -159,6 +159,7 @@ > > > > > > > > ;; The `.insn' pseudo-op. > > > > UNSPEC_INSN_PSEUDO > > > > + UNSPEC_JRHB > > > > ]) > > > > > > > > (define_constants > > > > @@ -6660,6 +6661,20 @@ > > > > [(set_attr "type" "jump") > > > > (set_attr "mode" "none")]) > > > > > > > > +;; Insn to clear execution and instruction hazards while returning. > > > > +;; However, it doesn't clear hazards created by the insn in its delay > > > > slot. > > > > +;; Thus, explicitly place a nop in its delay slot. > > > > + > > > > +(define_insn "mips_hb_return_internal" > > > > + [(return) > > > > + (unspec_volatile [(match_operand 0 "pmode_register_operand" "")] > > > > + UNSPEC_JRHB)] > > > > + "" > > > > + { > > > > + return "%(jr.hb\t$31%/%)"; > > > > + } > > > > + [(set_attr "insn_count" "2")]) > > > > + > > > > ;; Normal return. > > > > > > > > (define_insn "<optab>_internal" > > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index > > > > 49df8e6dc38..8d2a0a23af6 100644 > > > > --- a/gcc/doc/extend.texi > > > > +++ b/gcc/doc/extend.texi > > > > @@ -5540,6 +5540,12 @@ On MIPS targets, you can use the > > > > @code{nocompression} function attribute to locally turn off > > > > MIPS16 and microMIPS code generation. This attribute overrides > > > > the @option{-mips16} and @option{-mmicromips} options on the > > > > command > > > line (@pxref{MIPS Options}). > > > > + > > > > +@item use_hazard_barrier_return > > > > +@cindex @code{use_hazard_barrier_return} function attribute, MIPS > > > > +This function attribute instructs the compiler to generate a > > > > +hazard barrier return that clears all execution and instruction > > > > +hazards while returning, instead of generating a normal return > instruction. > > > > @end table > > > > > > > > @node MSP430 Function Attributes > > > > diff --git > > > > a/gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute.c > > > > b/gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute.c > > > > new file mode 100644 > > > > index 00000000000..3575af44dcd > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute. > > > > +++ c > > > > @@ -0,0 +1,20 @@ > > > > +/* Test attribute for clearing hazards while returning. */ > > > > +/* { dg-do compile } */ > > > > +/* { dg-options "isa_rev>=2 -mno-mips16" } */ > > > > + > > > > +extern int bar (); > > > > + > > > > +static int __attribute__ ((use_hazard_barrier_return)) > > > > +foo0 () > > > > +{ > > > > + return bar (); > > > > +} > > > > + > > > > +int > > > > +foo1 () > > > > +{ > > > > + return foo0 (); > > > > +} > > > > + > > > > +/* { dg-final { scan-assembler "foo0:" } } */ > > > > +/* { dg-final { scan-assembler-times "\tjr.hb\t\\\$31\n\tnop\\n" > > > > +1 } } */ > > > > -- > > > > 2.17.1
Ping.
