Please find the updated patch below. I hope I've covered everything.
I've added the test for inline restriction, could you check if I got all the
options correct?
Changelog:
2017-06-23 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 tests.
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi (revision 246899)
+++ gcc/doc/extend.texi (working copy)
@@ -4496,6 +4496,12 @@ On MIPS targets, you can use the @code{nocompressi
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
Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md (revision 246899)
+++ gcc/config/mips/mips.md (working copy)
@@ -156,6 +156,9 @@
;; The `.insn' pseudo-op.
UNSPEC_INSN_PSEUDO
+
+ ;; Hazard barrier return.
+ UNSPEC_JRHB
])
(define_constants
@@ -6578,6 +6581,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"
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c (revision 246899)
+++ gcc/config/mips/mips.c (working copy)
@@ -615,6 +615,7 @@ static const struct attribute_spec mips_attribute_
mips_handle_use_shadow_register_set_attr, false },
{ "keep_interrupts_masked", 0, 0, false, true, true, NULL, false },
{ "use_debug_exception_return", 0, 0, false, true, true, NULL, false },
+ { "use_hazard_barrier_return", 0, 0, true, false, false, NULL, false },
{ NULL, 0, 0, false, false, false, NULL, false }
};
@@ -1275,6 +1276,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. */
@@ -1460,6 +1471,21 @@ mips_can_inline_p (tree caller, tree callee)
return default_target_can_inline_p (caller, callee);
}
+/* Implement TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P.
+
+ A function reqeuesting 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)
+{
+ if (mips_use_hazard_barrier_return_p (decl))
+ return false;
+ return true;
+}
+
/* Handle an "interrupt" attribute with an optional argument. */
static tree
@@ -7863,6 +7889,17 @@ mips_function_ok_for_sibcall (tree decl, tree exp
&& !targetm.binds_local_p (decl))
return false;
+ /* Functions that need to return with a hazard barrier cannot sibcall
because:
+
+ 1) Hazard barriers are not possible for direct jumps
+
+ 2) Despite an indirect jump with hazard barrier being possible we do
+ not use it so that the logic for generating a hazard barrier jump
+ can be contained within the epilogue handling. */
+
+ if (mips_use_hazard_barrier_return_p (current_function_decl))
+ return false;
+
/* Otherwise OK. */
return true;
}
@@ -10943,6 +10980,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");
+ 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 ();
@@ -12606,7 +12654,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. */
@@ -12647,6 +12696,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;
@@ -12705,6 +12759,11 @@ mips_can_use_return_insn (void)
if (cfun->machine->interrupt_handler_p)
return false;
+ /* Even if the function has a null epilogue we choose to only create hazard
+ barrier returns in the epilogue expansion for simplicity. */
+ if (cfun->machine->use_hazard_barrier_return_p)
+ return false;
+
if (!reload_completed)
return false;
@@ -22476,10 +22535,9 @@ mips_promote_function_mode (const_tree type ATTRIB
#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
Index: gcc/config/mips/mips.h
===================================================================
--- gcc/config/mips/mips.h (revision 246899)
+++ gcc/config/mips/mips.h (working copy)
@@ -3405,6 +3405,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 a hazard barrier return. */
+ bool use_hazard_barrier_return_p;
};
#endif
Index: gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute.c
===================================================================
--- gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute.c
(revision 0)
+++ gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute.c
(revision 0)
@@ -0,0 +1,20 @@
+/* Test attribute for clearing hazards while returning. */
+/* { dg-do compile } */
+/* { dg-options "isa_rev>=2 -mno-mips16" } */
+
+extern int bar ();
+
+int __attribute__ ((use_hazard_barrier_return))
+foo ()
+{
+ return bar ();
+}
+/* { dg-final { scan-assembler "\tjr.hb\t\\\$31\n\tnop\n" } } */
+
+/* Test to confirm that foo() is not inlined. */
+int
+foo1 ()
+{
+ return foo ();
+}
+/* { dg-final { scan-assembler
"((j|jal|bc|balc)\tfoo)|((jr|jalr|jrc|jalrc)\t\\\$25)" } } */
-----Original Message-----
From: Matthew Fortune
Sent: Wednesday, June 14, 2017 9:48 PM
To: Prachi Godbole; gcc-patches@gcc.gnu.org
Subject: RE: Add support for use_hazard_barrier_return function attribute
Prachi Godbole <prachi.godb...@imgtec.com> writes:
> Changelog:
>
> 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.
Sorry for not getting back to this after stage1 opened.
This looks like a great idea. I'm slightly concerned about what will happen if
code uses this attribute and is built by older tools. The problem being that
it will just get a warning and that may or may not be strong enough for a user
to notice they did not get a jr.hb and then go on to get weird runtime failures.
That said we don't have this kind of feature detection for all the new
attributes relating to interrupts so perhaps we just leave this to -Werror
and/or configure time detection of the feature.
No major issues with this just a little cleanup...
> Index: gcc/doc/extend.texi
> ===================================================================
> --- gcc/doc/extend.texi (revision 246899)
> +++ gcc/doc/extend.texi (working copy)
> @@ -4496,6 +4496,12 @@ On MIPS targets, you can use the
> @code{nocompressi 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 hazard
> +barrier return
Missing an 'a':
... generate a hazard barrier return...
Documentation normally wraps at 74 columns which makes these lines a bit long.
> +that clears all execution and instruction hazards while returning,
> +instead of generating a normal return instruction.
> @end table
>
> @node MSP430 Function Attributes
> Index: gcc/config/mips/mips.md
> ===================================================================
> --- gcc/config/mips/mips.md (revision 246899)
> +++ gcc/config/mips/mips.md (working copy)
> @@ -156,6 +156,7 @@
>
> ;; The `.insn' pseudo-op.
> UNSPEC_INSN_PSEUDO
> + UNSPEC_JRHB
Add a comment on what the unspec is.
> ])
>
> (define_constants
> @@ -6578,6 +6579,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"
> Index: gcc/config/mips/mips.c
> ===================================================================
> --- gcc/config/mips/mips.c (revision 246899)
> +++ gcc/config/mips/mips.c (working copy)
> @@ -615,6 +615,7 @@ static const struct attribute_spec mips_attribute_
> mips_handle_use_shadow_register_set_attr, false },
> { "keep_interrupts_masked", 0, 0, false, true, true, NULL, false },
> { "use_debug_exception_return", 0, 0, false, true, true, NULL,
> false },
> + { "use_hazard_barrier_return", 0, 0, true, false, false, NULL,
> + false },
> { NULL, 0, 0, false, false, false, NULL, false }
> };
>
>
> @@ -1275,6 +1276,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 (tree decl)
Make this a const_tree so you don't have to const_cast.
> +{
> + return lookup_attribute ("use_hazard_barrier_return",
> + DECL_ATTRIBUTES (decl)) != NULL; }
> +
DECL_ATTRIBUTES is indented 1 space too many.
> /* Return the set of compression modes that are explicitly required
> by the attributes in ATTRIBUTES. */
>
> @@ -1460,6 +1471,21 @@ mips_can_inline_p (tree caller, tree callee)
> return default_target_can_inline_p (caller, callee); }
>
> +/* Implement TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P.
> +
> + A function reqeuesting 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) {
> + if (mips_use_hazard_barrier_return_p (const_cast<tree>(decl)))
Remove the const_cast as above.
> + return false;
> + return hook_bool_const_tree_true (decl); }
> +
Just return true, the various true/false hooks are only there to satisfy the
prototype required for the hook.
> /* Handle an "interrupt" attribute with an optional argument. */
>
> static tree
> @@ -7863,6 +7889,11 @@ mips_function_ok_for_sibcall (tree decl, tree exp
> && !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;
> +
I'd reword because this is not quite true:
Functions that need to return with a hazard barrier cannot sibcall
because:
1) Hazard barriers are not possible for direct jumps
2) Despite an indirect jump with hazard barrier being possible we do
not use it so that the logic for generating a hazard barrier jump
can be contained within the epilogue handling.
> /* Otherwise OK. */
> return true;
> }
> @@ -10943,6 +10974,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 return requires MIPS ISA to be R2 or
> +greater");
"hazard barrier returns require a MIPS32r2 processor or greater"
> + else if (TARGET_MIPS16)
> + error ("hazard barrier return cannot be used with MIPS16
> +functions");
"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 ();
> @@ -12606,7 +12648,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. */ @@ -12647,6 +12690,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;
> @@ -12705,6 +12753,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. */
Even if the function has a null epilogue we choose to only create hazard
barrier returns in the epilogue expansion for simplicity.
> + if (cfun->machine->use_hazard_barrier_return_p)
> + return false;
> +
> if (!reload_completed)
> return false;
>
> @@ -22476,10 +22529,9 @@ mips_promote_function_mode (const_tree type
> ATTRIB
>
> #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
> Index: gcc/config/mips/mips.h
> ===================================================================
> --- gcc/config/mips/mips.h (revision 246899)
> +++ gcc/config/mips/mips.h (working copy)
> @@ -3405,6 +3405,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. */
... generate a hazard barrier...
> + bool use_hazard_barrier_return_p;
> };
> #endif
>
> Index: gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute.c
> ===================================================================
> --- gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute.c
> (revision 0)
> +++ gcc/testsuite/gcc.target/mips/hazard-barrier-return-attribute.c
> (revision 0)
> @@ -0,0 +1,13 @@
> +/* Test attribute for clearing hazards while returning. */
> +/* { dg-do compile } */
> +/* { dg-options "isa_rev>=2 -mno-mips16" } */
> +
> +extern int bar ();
> +
> +int __attribute__ ((use_hazard_barrier_return)) foo () {
> + return bar ();
> +}
> +/* { dg-final { scan-assembler "\tjr.hb\t$31\n" } } */
> +/* { dg-final { scan-assembler "\tnop\n" } } */
The separate tests here don't guarantee the nop is in the delay slot, it needs
to check for both in one scan-assembler to do that. It will be something like
this (not tested):
> +/* { dg-final { scan-assembler "\tjr.hb\t$31\n\tnop\n" } } */
Can you extend this test to also cover the inlining restriction? I think the
test already covers sibcalls as it would normally have been a sibcall.
The extra bit of test will be something like:
int
another ()
{
return foo ();
}
/* { dg-final { scan-assembler "(j|jal|bc|balc)\tfoo" } } */
Please post the updated patch but I'm happy for you to commit if you understand
all my comments.
Thanks,
Matthew
