RE: Add support for use_hazard_barrier_return function attribute

[Maciej W. Rozycki]

On Thu, 6 Jul 2017, Matthew Fortune wrote:

> >  Nothing wrong with your proposed change, but overall I wonder if (as a
> > follow-up change) we could find a nonintrusive way to have this pattern
> > (and `clear_hazard_' as well) produce JRS.HB rather than JR.HB in
> > microMIPS compilations, as using the 32-bit delay-slot NOP encoding
> > where the 16-bit one would do is obviously a tiny, but completely
> > unnecessary code space loss (and we do care about code space losses in
> > microMIPS compilations; conserving space is the very purpose of the
> > microMIPS ISA after all).
> > 
> >  Of course it wouldn't do if we rewrote the instruction pattern as
> > "%(jr%!.hb\t$31%/%)" here, because the NOP that follows would have to
> > come from an RTL instruction for `%!' to have any effect.  But perhaps
> > we could emit RTL instead somehow rather than hardcoding the NOP with
> > `%/'?
> 
> I think this case is so specialist we can safely just switch to writing
> out the NOP directly in the output pattern just keeping the %(%) for
> noreorder. This code will have to be reworked with microMIPSR6 when
> submitted so it can be handled then; good spot to use jrs.hb.

 It does not matter for `%!' whether you use `%/' or spell out `nop' 
literally.  I was more concerned about getting the instruction count 
correctly, which would be 1.5 for the JRS.HB case, however I think you can 
just set the `length' attribute directly, to 6.

 Still the issue of having separate almost identical patterns remains, as 
barring the use of `%!' I think you'll need to qualify them with 
TARGET_MICROMIPS and !TARGET_MICROMIPS respectively, to have different 
instruction mnemonics.  In this case I think you could write (untested):

(define_insn "mips_hb_return_internal"
  [(return)
   (unspec_volatile [(match_operand 0 "pmode_register_operand" ",")]
   UNSPEC_JRHB)]
  ""
  "@
   %(jrs.hb\t$31%/%)
   %(jr.hb\t$31%/%)"
  [(set_attr "compression" "micromips,*")
   (set_attr "length" "6,8")])

however the equivalent for `clear_hazard_' would be rather horrible 
(OTOH eventually it should use ADDIUPC in its SImode microMIPS variant, so 
perhaps this is acceptable as we'll have multiple different sequences 
anyway).

 For microMIPSr6 we'll then just have another variant with no delay slot.

  Maciej

RE: Add support for use_hazard_barrier_return function attribute

[Matthew Fortune]

Maciej Rozycki  writes:
> On Fri, 23 Jun 2017, Prachi Godbole wrote:
> 
> > Index: gcc/config/mips/mips.md
> > ===
> > --- gcc/config/mips/mips.md (revision 246899)
> > +++ gcc/config/mips/mips.md (working copy)
> > @@ -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 "_internal"
> 
>  Nothing wrong with your proposed change, but overall I wonder if (as a
> follow-up change) we could find a nonintrusive way to have this pattern
> (and `clear_hazard_' as well) produce JRS.HB rather than JR.HB in
> microMIPS compilations, as using the 32-bit delay-slot NOP encoding
> where the 16-bit one would do is obviously a tiny, but completely
> unnecessary code space loss (and we do care about code space losses in
> microMIPS compilations; conserving space is the very purpose of the
> microMIPS ISA after all).
> 
>  Of course it wouldn't do if we rewrote the instruction pattern as
> "%(jr%!.hb\t$31%/%)" here, because the NOP that follows would have to
> come from an RTL instruction for `%!' to have any effect.  But perhaps
> we could emit RTL instead somehow rather than hardcoding the NOP with
> `%/'?

I think this case is so specialist we can safely just switch to writing
out the NOP directly in the output pattern just keeping the %(%) for
noreorder. This code will have to be reworked with microMIPSR6 when
submitted so it can be handled then; good spot to use jrs.hb.

Thanks,
Matthew

RE: Add support for use_hazard_barrier_return function attribute

[Matthew Fortune]

Prachi Godbole  writes:
> 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?

I think the test is probably good enough. It is a little too forgiving due
to handling the indirect call case to foo which could just detect an
indirect call from foo to bar (the placement of a scan-assembler in the
.c file has no impact on where in the generated output it will match in
the corresponding .s). Given that the test would fail appropriately on
a bare metal configuration (which is where this is likely to be most
useful) then I think that is sufficient.

Watch out for the long lines in comments. There is one that is hitting
80cols noted below to tweak before committing.

> Changelog:
> 
> 2017-06-23  Prachi Godbole  
> 
> 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.

OK to commit.

> ===
> --- gcc/config/mips/mips.c(revision 246899)
> +++ gcc/config/mips/mips.c(working copy)
> @@ -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:

Long line for a comment above.

> +
> + 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;
>  }

Thanks for the new feature!

Matthew

RE: Add support for use_hazard_barrier_return function attribute

[Maciej W. Rozycki]

On Fri, 23 Jun 2017, Prachi Godbole wrote:

> Index: gcc/config/mips/mips.md
> ===
> --- gcc/config/mips/mips.md   (revision 246899)
> +++ gcc/config/mips/mips.md   (working copy)
> @@ -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 "_internal"

 Nothing wrong with your proposed change, but overall I wonder if (as a 
follow-up change) we could find a nonintrusive way to have this pattern 
(and `clear_hazard_' as well) produce JRS.HB rather than JR.HB in 
microMIPS compilations, as using the 32-bit delay-slot NOP encoding where 
the 16-bit one would do is obviously a tiny, but completely unnecessary 
code space loss (and we do care about code space losses in microMIPS 
compilations; conserving space is the very purpose of the microMIPS ISA 
after all).

 Of course it wouldn't do if we rewrote the instruction pattern as 
"%(jr%!.hb\t$31%/%)" here, because the NOP that follows would have to come 
from an RTL instruction for `%!' to have any effect.  But perhaps we could 
emit RTL instead somehow rather than hardcoding the NOP with `%/'?

  Maciej

RE: Add support for use_hazard_barrier_return function attribute

[Prachi Godbole]

nds_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 = >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_inter

RE: Add support for use_hazard_barrier_return function attribute

[Matthew Fortune]

Prachi Godbole  writes:
> Changelog:
> 
> 2017-04-25  Prachi Godbole  
> 
> 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 "_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