Add support for use_hazard_barrier_return function attribute

This patch adds support for function attribute  __attribute__ ((use_hazard_barrier_return)). The attribute will generate hazard barrier return (jr.hb) instead of a normal return instruction.

Changelog:

2017-04-25  Prachi Godbole  <prachi.godbole@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.


Ok for stage1?

Regards,
Prachi

Prachi Godbole <Prachi.Godbole@imgtec.com> writes:
> Changelog:
> 
> 2017-04-25  Prachi Godbole  <prachi.godbole@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