[2/6,RS6000] rs6000_output_indirect_call

Like the last patch for external calls, now handle most assembly code
for indirect calls in one place.  The patch also merges some insns,
correcting some !rs6000_speculate_indirect_jumps cases branching to
LR, which don't require a speculation barrier.

	* config/rs6000/rs6000-protos.h (rs6000_output_indirect_call): Declare.
	* config/rs6000/rs6000.c (rs6000_output_indirect_call): New function.
	* config/rs6000/rs6000.md (call_indirect_nonlocal_sysv): Use
	rs6000_output_indirect_call.
	(call_value_indirect_nonlocal_sysv, sibcall_nonlocal_sysv): Likewise.
	(call_indirect_aix, call_value_indirect_aix,
	call_indirect_elfv2, call_value_indirect_elfv2): Likewise, and
	handle both speculation and non-speculation cases.
	(call_indirect_aix_nospec, call_value_indirect_aix_nospec): Delete.
	(call_indirect_elfv2_nospec, call_value_indirect_elfv2_nospec): Delete.

On 11/6/18 11:37 PM, Alan Modra wrote:
> Like the last patch for external calls, now handle most assembly code
> for indirect calls in one place.  The patch also merges some insns,
> correcting some !rs6000_speculate_indirect_jumps cases branching to
> LR, which don't require a speculation barrier.
>
> 	* config/rs6000/rs6000-protos.h (rs6000_output_indirect_call): Declare.
> 	* config/rs6000/rs6000.c (rs6000_output_indirect_call): New function.
> 	* config/rs6000/rs6000.md (call_indirect_nonlocal_sysv): Use
> 	rs6000_output_indirect_call.
> 	(call_value_indirect_nonlocal_sysv, sibcall_nonlocal_sysv): Likewise.
> 	(call_indirect_aix, call_value_indirect_aix,
> 	call_indirect_elfv2, call_value_indirect_elfv2): Likewise, and
> 	handle both speculation and non-speculation cases.
> 	(call_indirect_aix_nospec, call_value_indirect_aix_nospec): Delete.
> 	(call_indirect_elfv2_nospec, call_value_indirect_elfv2_nospec): Delete.
>
> diff --git a/gcc/config/rs6000/rs6000-protos.h b/gcc/config/rs6000/rs6000-protos.h
> index f1a421dde16..493cfe6ba2b 100644
> --- a/gcc/config/rs6000/rs6000-protos.h
> +++ b/gcc/config/rs6000/rs6000-protos.h
> @@ -112,6 +112,7 @@ extern void rs6000_output_function_entry (FILE *, const char *);
>  extern void print_operand (FILE *, rtx, int);
>  extern void print_operand_address (FILE *, rtx);
>  extern const char *rs6000_output_call (rtx *, unsigned int, bool, const char *);
> +extern const char *rs6000_output_indirect_call (rtx *, unsigned int, bool);
>  extern enum rtx_code rs6000_reverse_condition (machine_mode,
>  					       enum rtx_code);
>  extern rtx rs6000_emit_eqne (machine_mode, rtx, rtx, rtx);
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index b22cae55a0d..bf1551746d5 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -21411,6 +21411,69 @@ rs6000_output_call (rtx *operands, unsigned int fun, bool sibcall,
>    return str;
>  }
>
> +/* As above, for indirect calls.  */
> +
> +const char *
> +rs6000_output_indirect_call (rtx *operands, unsigned int fun, bool sibcall)
> +{
> +  /* -Wformat-overflow workaround, without which gcc thinks that %u
> +      might produce 10 digits.  FUN is 0 or 1 as of 2018-03.  */
> +  gcc_assert (fun <= 6);
> +
> +  static char str[144];
> +  const char *ptrload = TARGET_64BIT ? "d" : "wz";
> +
> +  bool speculate = (rs6000_speculate_indirect_jumps
> +		    || (REG_P (operands[fun])
> +			&& REGNO (operands[fun]) == LR_REGNO));

Wouldn't hurt to have a comment here, indicating that we only have to generate
inefficient, speculation-inhibiting code when speculation via the count cache
has been disabled by switch, and this doesn't apply to indirect calls via the
link register.  This changes behavior of the code from before, but appears to
be safe and more correct.

> +
> +  if (DEFAULT_ABI == ABI_AIX)
> +    {
> +      if (speculate)
> +	sprintf (str,
> +		 "l%s 2,%%%u\n\t"
> +		 "b%%T%ul\n\t"
> +		 "l%s 2,%%%u(1)",
> +		 ptrload, fun + 2, fun, ptrload, fun + 3);
> +      else
> +	sprintf (str,
> +		 "crset 2\n\t"
> +		 "l%s 2,%%%u\n\t"
> +		 "beq%%T%ul-\n\t"
> +		 "l%s 2,%%%u(1)",
> +		 ptrload, fun + 2, fun, ptrload, fun + 3);
> +    }
> +  else if (DEFAULT_ABI == ABI_ELFv2)
> +    {
> +      if (speculate)
> +	sprintf (str,
> +		 "b%%T%ul\n\t"
> +		 "l%s 2,%%%u(1)",
> +		 fun, ptrload, fun + 2);
> +      else
> +	sprintf (str,
> +		 "crset 2\n\t"
> +		 "beq%%T%ul-\n\t"
> +		 "l%s 2,%%%u(1)",
> +		 fun, ptrload, fun + 2);
> +    }
> +  else if (DEFAULT_ABI == ABI_V4)
> +    {
> +      if (speculate)
> +	sprintf (str,
> +		 "b%%T%u%s",
> +		 fun, "l" + sibcall);

It's not at all clear to me what {"l" + sibcall} is doing here.
Whatever it is, it's clever enough that it warrants a comment... :-)
Does adding "l" to false result in the null string?  Is that
standard?

> +      else
> +	sprintf (str,
> +		 "crset 2\n\t"
> +		 "beq%%T%u%s-%s",
> +		 fun, "l" + sibcall, sibcall ? "\n\tb $" : "");

And similar...

> +    }
> +  else
> +    gcc_unreachable ();
> +  return str;
> +}
> +
>  #if defined (HAVE_GAS_HIDDEN) && !TARGET_MACHO
>  /* Emit an assembler directive to set symbol visibility for DECL to
>     VISIBILITY_TYPE.  */
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 52088fdfbdb..9d9e29d12eb 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -10540,11 +10540,7 @@ (define_insn "*call_indirect_nonlocal_sysv<mode>"
>    else if (INTVAL (operands[2]) & CALL_V4_CLEAR_FP_ARGS)
>      output_asm_insn ("creqv 6,6,6", operands);
>
> -  if (rs6000_speculate_indirect_jumps
> -      || which_alternative == 1 || which_alternative == 3)
> -    return "b%T0l";
> -  else
> -    return "crset 2\;beq%T0l-";
> +  return rs6000_output_indirect_call (operands, 0, false);

Looks like this breaks Darwin?  This pattern matches for DEFAULT_ABI == ABI_DARWIN
but rs6000_output_indirect_call will hit gcc_unreachable() for that ABI.  

>  }
>    [(set_attr "type" "jmpreg,jmpreg,jmpreg,jmpreg")
>     (set_attr_alternative "length"
> @@ -10630,11 +10626,7 @@ (define_insn "*call_value_indirect_nonlocal_sysv<mode>"
>    else if (INTVAL (operands[3]) & CALL_V4_CLEAR_FP_ARGS)
>      output_asm_insn ("creqv 6,6,6", operands);
>
> -  if (rs6000_speculate_indirect_jumps
> -      || which_alternative == 1 || which_alternative == 3)
> -    return "b%T1l";
> -  else
> -    return "crset 2\;beq%T1l-";
> +  return rs6000_output_indirect_call (operands, 1, false);

Here also...

Otherwise this looks correct to me, though I can't approve it.  It's a nice cleanup!

Thanks,
Bill

>  }
>    [(set_attr "type" "jmpreg,jmpreg,jmpreg,jmpreg")
>     (set_attr_alternative "length"
> @@ -10765,21 +10757,16 @@ (define_insn "*call_indirect_aix<mode>"
>     (use (match_operand:P 2 "memory_operand" "<ptrm>,<ptrm>"))
>     (set (reg:P TOC_REGNUM) (unspec:P [(match_operand:P 3 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
>     (clobber (reg:P LR_REGNO))]
> -  "DEFAULT_ABI == ABI_AIX && rs6000_speculate_indirect_jumps"
> -  "<ptrload> 2,%2\;b%T0l\;<ptrload> 2,%3(1)"
> -  [(set_attr "type" "jmpreg")
> -   (set_attr "length" "12")])
> -
> -(define_insn "*call_indirect_aix<mode>_nospec"
> -  [(call (mem:SI (match_operand:P 0 "register_operand" "c,*l"))
> -	 (match_operand 1 "" "g,g"))
> -   (use (match_operand:P 2 "memory_operand" "<ptrm>,<ptrm>"))
> -   (set (reg:P TOC_REGNUM) (unspec:P [(match_operand:P 3 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
> -   (clobber (reg:P LR_REGNO))]
> -  "DEFAULT_ABI == ABI_AIX && !rs6000_speculate_indirect_jumps"
> -  "crset 2\;<ptrload> 2,%2\;beq%T0l-\;<ptrload> 2,%3(1)"
> +  "DEFAULT_ABI == ABI_AIX"
> +{
> +  return rs6000_output_indirect_call (operands, 0, false);
> +}
>    [(set_attr "type" "jmpreg")
> -   (set_attr "length" "16")])
> +   (set (attr "length")
> +	(if_then_else (and (match_test "!rs6000_speculate_indirect_jumps")
> +			   (match_test "which_alternative != 1"))
> +		      (const_string "16")
> +		      (const_string "12")))])
>
>  (define_insn "*call_value_indirect_aix<mode>"
>    [(set (match_operand 0 "" "")
> @@ -10788,22 +10775,16 @@ (define_insn "*call_value_indirect_aix<mode>"
>     (use (match_operand:P 3 "memory_operand" "<ptrm>,<ptrm>"))
>     (set (reg:P TOC_REGNUM) (unspec:P [(match_operand:P 4 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
>     (clobber (reg:P LR_REGNO))]
> -  "DEFAULT_ABI == ABI_AIX && rs6000_speculate_indirect_jumps"
> -  "<ptrload> 2,%3\;b%T1l\;<ptrload> 2,%4(1)"
> -  [(set_attr "type" "jmpreg")
> -   (set_attr "length" "12")])
> -
> -(define_insn "*call_value_indirect_aix<mode>_nospec"
> -  [(set (match_operand 0 "" "")
> -	(call (mem:SI (match_operand:P 1 "register_operand" "c,*l"))
> -	      (match_operand 2 "" "g,g")))
> -   (use (match_operand:P 3 "memory_operand" "<ptrm>,<ptrm>"))
> -   (set (reg:P TOC_REGNUM) (unspec:P [(match_operand:P 4 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
> -   (clobber (reg:P LR_REGNO))]
> -  "DEFAULT_ABI == ABI_AIX && !rs6000_speculate_indirect_jumps"
> -  "crset 2\;<ptrload> 2,%3\;beq%T1l-\;<ptrload> 2,%4(1)"
> +  "DEFAULT_ABI == ABI_AIX"
> +{
> +  return rs6000_output_indirect_call (operands, 1, false);
> +}
>    [(set_attr "type" "jmpreg")
> -   (set_attr "length" "16")])
> +   (set (attr "length")
> +	(if_then_else (and (match_test "!rs6000_speculate_indirect_jumps")
> +			   (match_test "which_alternative != 1"))
> +		      (const_string "16")
> +		      (const_string "12")))])
>
>  ;; Call to indirect functions with the ELFv2 ABI.
>  ;; Operand0 is the addresss of the function to call
> @@ -10814,21 +10795,16 @@ (define_insn "*call_indirect_elfv2<mode>"
>  	 (match_operand 1 "" "g,g"))
>     (set (reg:P TOC_REGNUM) (unspec:P [(match_operand:P 2 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
>     (clobber (reg:P LR_REGNO))]
> -  "DEFAULT_ABI == ABI_ELFv2 && rs6000_speculate_indirect_jumps"
> -  "b%T0l\;<ptrload> 2,%2(1)"
> -  [(set_attr "type" "jmpreg")
> -   (set_attr "length" "8")])
> -
> -;; Variant with deliberate misprediction.
> -(define_insn "*call_indirect_elfv2<mode>_nospec"
> -  [(call (mem:SI (match_operand:P 0 "register_operand" "c,*l"))
> -	 (match_operand 1 "" "g,g"))
> -   (set (reg:P TOC_REGNUM) (unspec:P [(match_operand:P 2 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
> -   (clobber (reg:P LR_REGNO))]
> -  "DEFAULT_ABI == ABI_ELFv2 && !rs6000_speculate_indirect_jumps"
> -  "crset 2\;beq%T0l-\;<ptrload> 2,%2(1)"
> +  "DEFAULT_ABI == ABI_ELFv2"
> +{
> +  return rs6000_output_indirect_call (operands, 0, false);
> +}
>    [(set_attr "type" "jmpreg")
> -   (set_attr "length" "12")])
> +   (set (attr "length")
> +	(if_then_else (and (match_test "!rs6000_speculate_indirect_jumps")
> +			   (match_test "which_alternative != 1"))
> +		      (const_string "12")
> +		      (const_string "8")))])
>
>  (define_insn "*call_value_indirect_elfv2<mode>"
>    [(set (match_operand 0 "" "")
> @@ -10836,22 +10812,16 @@ (define_insn "*call_value_indirect_elfv2<mode>"
>  	      (match_operand 2 "" "g,g")))
>     (set (reg:P TOC_REGNUM) (unspec:P [(match_operand:P 3 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
>     (clobber (reg:P LR_REGNO))]
> -  "DEFAULT_ABI == ABI_ELFv2 && rs6000_speculate_indirect_jumps"
> -  "b%T1l\;<ptrload> 2,%3(1)"
> -  [(set_attr "type" "jmpreg")
> -   (set_attr "length" "8")])
> -
> -; Variant with deliberate misprediction.
> -(define_insn "*call_value_indirect_elfv2<mode>_nospec"
> -  [(set (match_operand 0 "" "")
> -	(call (mem:SI (match_operand:P 1 "register_operand" "c,*l"))
> -	      (match_operand 2 "" "g,g")))
> -   (set (reg:P TOC_REGNUM) (unspec:P [(match_operand:P 3 "const_int_operand" "n,n")] UNSPEC_TOCSLOT))
> -   (clobber (reg:P LR_REGNO))]
> -  "DEFAULT_ABI == ABI_ELFv2 && !rs6000_speculate_indirect_jumps"
> -  "crset 2\;beq%T1l-\;<ptrload> 2,%3(1)"
> +  "DEFAULT_ABI == ABI_ELFv2"
> +{
> +  return rs6000_output_indirect_call (operands, 1, false);
> +}
>    [(set_attr "type" "jmpreg")
> -   (set_attr "length" "12")])
> +   (set (attr "length")
> +	(if_then_else (and (match_test "!rs6000_speculate_indirect_jumps")
> +			   (match_test "which_alternative != 1"))
> +		      (const_string "12")
> +		      (const_string "8")))])
>
>  ;; Call subroutine returning any type.
>  (define_expand "untyped_call"
> @@ -11020,13 +10990,7 @@ (define_insn "*sibcall_nonlocal_sysv<mode>"
>      output_asm_insn ("creqv 6,6,6", operands);
>
>    if (which_alternative >= 2)
> -    {
> -      if (rs6000_speculate_indirect_jumps)
> -	return "b%T0";
> -      else
> -	/* Can use CR0 since it is volatile across sibcalls.  */
> -	return "crset 2\;beq%T0-\;b $";
> -    }
> +    return rs6000_output_indirect_call (operands, 0, true);
>    else
>      return rs6000_output_call (operands, 0, true, "");
>  }
>

On Mon, Nov 12, 2018 at 01:44:08PM -0600, Bill Schmidt wrote:
> On 11/6/18 11:37 PM, Alan Modra wrote:
> > +		 fun, "l" + sibcall);
> 
> It's not at all clear to me what {"l" + sibcall} is doing here.

It's an ancient C programmer's trick, from the days when most
compilers didn't optimize too well.  I think I found it first in the
nethack sources.  :-)

> Whatever it is, it's clever enough that it warrants a comment... :-)
> Does adding "l" to false result in the null string?  Is that
> standard?

"l" results in a "const char*" pointing at 0x6c,0 bytes in memory
(assuming ascii).  Adding "true" to that implicitly converts "true" to
1 and thus a "const char*" pointing at a NUL byte.  All completely
standard, even in that new fangled C++ thingy.

A comment is as much needed as count++; needs /* add one to count. */.
If it bothers people I'll write:  sibcall ? "" : "l".

Hah, even the latest gcc doesn't optimize the conditional expression
down to "l" + sibcall.  Check out the code generated for

const char *
f1 (bool x)
{
  return "a" + x;
}

const char *
f2 (bool x)
{
  return x ? "" : "b";
}

> > --- a/gcc/config/rs6000/rs6000.md
> > +++ b/gcc/config/rs6000/rs6000.md
> > @@ -10540,11 +10540,7 @@ (define_insn "*call_indirect_nonlocal_sysv<mode>"
> >    else if (INTVAL (operands[2]) & CALL_V4_CLEAR_FP_ARGS)
> >      output_asm_insn ("creqv 6,6,6", operands);
> >
> > -  if (rs6000_speculate_indirect_jumps
> > -      || which_alternative == 1 || which_alternative == 3)
> > -    return "b%T0l";
> > -  else
> > -    return "crset 2\;beq%T0l-";
> > +  return rs6000_output_indirect_call (operands, 0, false);
> 
> Looks like this breaks Darwin?  This pattern matches for DEFAULT_ABI == ABI_DARWIN
> but rs6000_output_indirect_call will hit gcc_unreachable() for that ABI.  

Hmm, yes, thanks for pointing that one out.  I took too much notice of
the pattern name.

Segher, would you like me to repost the series with accumulated fixes,
I mean before you review the rest of the series?

On Tue, Nov 13, 2018 at 10:44:24AM +1030, Alan Modra wrote:
> On Mon, Nov 12, 2018 at 01:44:08PM -0600, Bill Schmidt wrote:
> > On 11/6/18 11:37 PM, Alan Modra wrote:
> > > +		 fun, "l" + sibcall);
> > 
> > It's not at all clear to me what {"l" + sibcall} is doing here.
> 
> It's an ancient C programmer's trick, from the days when most
> compilers didn't optimize too well.  I think I found it first in the
> nethack sources.  :-)
> 
> > Whatever it is, it's clever enough that it warrants a comment... :-)
> > Does adding "l" to false result in the null string?  Is that
> > standard?
> 
> "l" results in a "const char*" pointing at 0x6c,0 bytes in memory
> (assuming ascii).  Adding "true" to that implicitly converts "true" to
> 1 and thus a "const char*" pointing at a NUL byte.  All completely
> standard, even in that new fangled C++ thingy.
> 
> A comment is as much needed as count++; needs /* add one to count. */.
> If it bothers people I'll write:  sibcall ? "" : "l".

That is nicer, esp. if you can propagate it a bit, simplify the separate
cases.

> Hah, even the latest gcc doesn't optimize the conditional expression
> down to "l" + sibcall.  Check out the code generated for
> 
> const char *
> f1 (bool x)
> {
>   return "a" + x;
> }
> 
> const char *
> f2 (bool x)
> {
>   return x ? "" : "b";
> }

It also doesn't do the opposite: it doesn't optimise

const char *f(_Bool x) { fputs("x" + x, stdout); }

(if x is true this does nothing).

> Segher, would you like me to repost the series with accumulated fixes,
> I mean before you review the rest of the series?

Yes please.


Segher