[v3,rs6000] Add -mspeculate-indirect-jumps option and implement non-speculating bctr / bctrl

Hi,

This patch supercedes v2: https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01204.html,
and fixes the problems noted in its review.  It also adds the test cases from
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01261.html and adjusts them according
to the results of the review.

There is more function to be provided in a future patch:  Sibling calls for all ABIs,
and indirect calls for non-ELFv2 ABIs.  I'm getting close on that, but I think it's
better to keep that separate at this point.

Bootstrapped and tested on powerpc64-linux-gnu and powerpc64le-linux-gnu with no
regressions.  Is this okay for trunk?

Thanks,
Bill


[gcc]

2018-01-15  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	* config/rs6000/rs6000.c (rs6000_opt_vars): Add entry for
	-mspeculate-indirect-jumps.
	* config/rs6000/rs6000.md (*call_indirect_elfv2<mode>): Disable
	for -mno-speculate-indirect-jumps.
	(*call_indirect_elfv2<mode>_nospec): New define_insn.
	(*call_value_indirect_elfv2<mode>): Disable for
	-mno-speculate-indirect-jumps.
	(*call_value_indirect_elfv2<mode>_nospec): New define_insn.
	(indirect_jump): Emit different RTL for
	-mno-speculate-indirect-jumps.
	(*indirect_jump<mode>): Disable for
	-mno-speculate-indirect-jumps.
	(*indirect_jump<mode>_nospec): New define_insn.
	(tablejump): Emit different RTL for
	-mno-speculate-indirect-jumps.
	(tablejumpsi): Disable for -mno-speculate-indirect-jumps.
	(tablejumpsi_nospec): New define_expand.
	(tablejumpdi): Disable for -mno-speculate-indirect-jumps.
	(tablejumpdi_nospec): New define_expand.
	(*tablejump<mode>_internal1): Disable for
	-mno-speculate-indirect-jumps.
	(*tablejump<mode>_internal1_nospec): New define_insn.
	* config/rs6000/rs6000.opt (mspeculate-indirect-jumps): New
	option.

[gcc/testsuite]

2018-01-15  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	* gcc.target/powerpc/safe-indirect-jump-1.c: New file.
	* gcc.target/powerpc/safe-indirect-jump-2.c: New file.
	* gcc.target/powerpc/safe-indirect-jump-3.c: New file.
	* gcc.target/powerpc/safe-indirect-jump-4.c: New file.
	* gcc.target/powerpc/safe-indirect-jump-5.c: New file.
	* gcc.target/powerpc/safe-indirect-jump-6.c: New file.

On Tue, Jan 16, 2018 at 12:09 AM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> Hi,
>
> This patch supercedes v2: https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01204.html,
> and fixes the problems noted in its review.  It also adds the test cases from
> https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01261.html and adjusts them according
> to the results of the review.
>
> There is more function to be provided in a future patch:  Sibling calls for all ABIs,
> and indirect calls for non-ELFv2 ABIs.  I'm getting close on that, but I think it's
> better to keep that separate at this point.
>
> Bootstrapped and tested on powerpc64-linux-gnu and powerpc64le-linux-gnu with no
> regressions.  Is this okay for trunk?

Did you consider simply removing the tablejump/casesi support so
expansion always
expands to a balanced tree?  At least if we have any knobs to tune we
should probably
tweak them away from the indirect jump using variants with
-mno-speculate-indirect-jumps,
right?

Performance optimization, so shouldn't block this patch - I just
thought I should probably
mention this.

Richard.

> Thanks,
> Bill
>
>
> [gcc]
>
> 2018-01-15  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
>
>         * config/rs6000/rs6000.c (rs6000_opt_vars): Add entry for
>         -mspeculate-indirect-jumps.
>         * config/rs6000/rs6000.md (*call_indirect_elfv2<mode>): Disable
>         for -mno-speculate-indirect-jumps.
>         (*call_indirect_elfv2<mode>_nospec): New define_insn.
>         (*call_value_indirect_elfv2<mode>): Disable for
>         -mno-speculate-indirect-jumps.
>         (*call_value_indirect_elfv2<mode>_nospec): New define_insn.
>         (indirect_jump): Emit different RTL for
>         -mno-speculate-indirect-jumps.
>         (*indirect_jump<mode>): Disable for
>         -mno-speculate-indirect-jumps.
>         (*indirect_jump<mode>_nospec): New define_insn.
>         (tablejump): Emit different RTL for
>         -mno-speculate-indirect-jumps.
>         (tablejumpsi): Disable for -mno-speculate-indirect-jumps.
>         (tablejumpsi_nospec): New define_expand.
>         (tablejumpdi): Disable for -mno-speculate-indirect-jumps.
>         (tablejumpdi_nospec): New define_expand.
>         (*tablejump<mode>_internal1): Disable for
>         -mno-speculate-indirect-jumps.
>         (*tablejump<mode>_internal1_nospec): New define_insn.
>         * config/rs6000/rs6000.opt (mspeculate-indirect-jumps): New
>         option.
>
> [gcc/testsuite]
>
> 2018-01-15  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
>
>         * gcc.target/powerpc/safe-indirect-jump-1.c: New file.
>         * gcc.target/powerpc/safe-indirect-jump-2.c: New file.
>         * gcc.target/powerpc/safe-indirect-jump-3.c: New file.
>         * gcc.target/powerpc/safe-indirect-jump-4.c: New file.
>         * gcc.target/powerpc/safe-indirect-jump-5.c: New file.
>         * gcc.target/powerpc/safe-indirect-jump-6.c: New file.
>
>
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 256364)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -36726,6 +36726,9 @@ static struct rs6000_opt_var const rs6000_opt_vars
>    { "sched-epilog",
>      offsetof (struct gcc_options, x_TARGET_SCHED_PROLOG),
>      offsetof (struct cl_target_option, x_TARGET_SCHED_PROLOG), },
> +  { "speculate-indirect-jumps",
> +    offsetof (struct gcc_options, x_rs6000_speculate_indirect_jumps),
> +    offsetof (struct cl_target_option, x_rs6000_speculate_indirect_jumps), },
>  };
>
>  /* Inner function to handle attribute((target("..."))) and #pragma GCC target
> Index: gcc/config/rs6000/rs6000.md
> ===================================================================
> --- gcc/config/rs6000/rs6000.md (revision 256364)
> +++ gcc/config/rs6000/rs6000.md (working copy)
> @@ -11222,11 +11222,22 @@
>          (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"
> +  "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 eq\;beq%T0l-\;<ptrload> 2,%2(1)"
> +  [(set_attr "type" "jmpreg")
> +   (set_attr "length" "12")])
> +
>  (define_insn "*call_value_indirect_elfv2<mode>"
>    [(set (match_operand 0 "" "")
>         (call (mem:SI (match_operand:P 1 "register_operand" "c,*l"))
> @@ -11233,11 +11244,22 @@
>               (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"
> +  "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 eq\;beq%T1l-\;<ptrload> 2,%3(1)"
> +  [(set_attr "type" "jmpreg")
> +   (set_attr "length" "12")])
>
>  ;; Call subroutine returning any type.
>  (define_expand "untyped_call"
> @@ -12917,15 +12939,34 @@
>    [(set_attr "type" "jmpreg")])
>
>  (define_expand "indirect_jump"
> -  [(set (pc) (match_operand 0 "register_operand"))])
> +  [(set (pc) (match_operand 0 "register_operand"))]
> + ""
> +{
> +  if (!rs6000_speculate_indirect_jumps) {
> +    rtx ccreg = gen_reg_rtx (CCmode);
> +    if (Pmode == DImode)
> +      emit_jump_insn (gen_indirect_jumpdi_nospec (operands[0], ccreg));
> +    else
> +      emit_jump_insn (gen_indirect_jumpsi_nospec (operands[0], ccreg));
> +    DONE;
> +  }
> +})
>
>  (define_insn "*indirect_jump<mode>"
>    [(set (pc)
>         (match_operand:P 0 "register_operand" "c,*l"))]
> -  ""
> +  "rs6000_speculate_indirect_jumps"
>    "b%T0"
>    [(set_attr "type" "jmpreg")])
>
> +(define_insn "indirect_jump<mode>_nospec"
> +  [(set (pc) (match_operand:P 0 "register_operand" "c,*l"))
> +   (clobber (match_operand:CC 1 "cc_reg_operand" "=y,y"))]
> +  "!rs6000_speculate_indirect_jumps"
> +  "crset %E1\;beq%T0- %1\;b ."
> +  [(set_attr "type" "jmpreg")
> +   (set_attr "length" "12")])
> +
>  ;; Table jump for switch statements:
>  (define_expand "tablejump"
>    [(use (match_operand 0))
> @@ -12933,9 +12974,27 @@
>    ""
>  {
>    if (TARGET_32BIT)
> -    emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
> +    {
> +      if (rs6000_speculate_indirect_jumps)
> +       emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
> +      else
> +       {
> +         rtx ccreg = gen_reg_rtx (CCmode);
> +         rtx jump = gen_tablejumpsi_nospec (operands[0], operands[1], ccreg);
> +         emit_jump_insn (jump);
> +       }
> +    }
>    else
> -    emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
> +    {
> +      if (rs6000_speculate_indirect_jumps)
> +       emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
> +      else
> +       {
> +         rtx ccreg = gen_reg_rtx (CCmode);
> +         rtx jump = gen_tablejumpdi_nospec (operands[0], operands[1], ccreg);
> +         emit_jump_insn (jump);
> +       }
> +    }
>    DONE;
>  })
>
> @@ -12946,7 +13005,7 @@
>     (parallel [(set (pc)
>                    (match_dup 3))
>               (use (label_ref (match_operand 1)))])]
> -  "TARGET_32BIT"
> +  "TARGET_32BIT && rs6000_speculate_indirect_jumps"
>  {
>    operands[0] = force_reg (SImode, operands[0]);
>    operands[2] = force_reg (SImode, gen_rtx_LABEL_REF (SImode, operands[1]));
> @@ -12953,6 +13012,21 @@
>    operands[3] = gen_reg_rtx (SImode);
>  })
>
> +(define_expand "tablejumpsi_nospec"
> +  [(set (match_dup 4)
> +       (plus:SI (match_operand:SI 0)
> +                (match_dup 3)))
> +   (parallel [(set (pc)
> +                  (match_dup 4))
> +             (use (label_ref (match_operand 1)))
> +             (clobber (match_operand 2))])]
> +  "TARGET_32BIT && !rs6000_speculate_indirect_jumps"
> +{
> +  operands[0] = force_reg (SImode, operands[0]);
> +  operands[3] = force_reg (SImode, gen_rtx_LABEL_REF (SImode, operands[1]));
> +  operands[4] = gen_reg_rtx (SImode);
> +})
> +
>  (define_expand "tablejumpdi"
>    [(set (match_dup 4)
>          (sign_extend:DI (match_operand:SI 0 "lwa_operand")))
> @@ -12962,7 +13036,7 @@
>     (parallel [(set (pc)
>                    (match_dup 3))
>               (use (label_ref (match_operand 1)))])]
> -  "TARGET_64BIT"
> +  "TARGET_64BIT && rs6000_speculate_indirect_jumps"
>  {
>    operands[2] = force_reg (DImode, gen_rtx_LABEL_REF (DImode, operands[1]));
>    operands[3] = gen_reg_rtx (DImode);
> @@ -12969,14 +13043,41 @@
>    operands[4] = gen_reg_rtx (DImode);
>  })
>
> +(define_expand "tablejumpdi_nospec"
> +  [(set (match_dup 5)
> +        (sign_extend:DI (match_operand:SI 0 "lwa_operand")))
> +   (set (match_dup 4)
> +       (plus:DI (match_dup 5)
> +                (match_dup 3)))
> +   (parallel [(set (pc)
> +                  (match_dup 4))
> +             (use (label_ref (match_operand 1)))
> +             (clobber (match_operand 2))])]
> +  "TARGET_64BIT && !rs6000_speculate_indirect_jumps"
> +{
> +  operands[3] = force_reg (DImode, gen_rtx_LABEL_REF (DImode, operands[1]));
> +  operands[4] = gen_reg_rtx (DImode);
> +  operands[5] = gen_reg_rtx (DImode);
> +})
> +
>  (define_insn "*tablejump<mode>_internal1"
>    [(set (pc)
>         (match_operand:P 0 "register_operand" "c,*l"))
>     (use (label_ref (match_operand 1)))]
> -  ""
> +  "rs6000_speculate_indirect_jumps"
>    "b%T0"
>    [(set_attr "type" "jmpreg")])
>
> +(define_insn "*tablejump<mode>_internal1_nospec"
> +  [(set (pc)
> +       (match_operand:P 0 "register_operand" "c,*l"))
> +   (use (label_ref (match_operand 1)))
> +   (clobber (match_operand:CC 2 "cc_reg_operand" "=y,y"))]
> +  "!rs6000_speculate_indirect_jumps"
> +  "crset %E2\;beq%T0- %2\;b ."
> +  [(set_attr "type" "jmpreg")
> +   (set_attr "length" "12")])
> +
>  (define_insn "nop"
>    [(unspec [(const_int 0)] UNSPEC_NOP)]
>    ""
> Index: gcc/config/rs6000/rs6000.opt
> ===================================================================
> --- gcc/config/rs6000/rs6000.opt        (revision 256364)
> +++ gcc/config/rs6000/rs6000.opt        (working copy)
> @@ -617,3 +617,8 @@ Use the given offset for addressing the stack-prot
>
>  TargetVariable
>  long rs6000_stack_protector_guard_offset = 0
> +
> +;; -mno-speculate-indirect-jumps adds deliberate misprediction to indirect
> +;; branches via the CTR.
> +mspeculate-indirect-jumps
> +Target Undocumented Var(rs6000_speculate_indirect_jumps) Init(1) Save
> Index: gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-1.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-1.c     (nonexistent)
> +++ gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-1.c     (working copy)
> @@ -0,0 +1,14 @@
> +/* { dg-do compile { target { powerpc64le-*-* } } } */
> +/* { dg-additional-options "-mno-speculate-indirect-jumps" } */
> +
> +/* Test for deliberate misprediction of indirect calls for ELFv2.  */
> +
> +extern int (*f)();
> +
> +int bar ()
> +{
> +  return (*f) ();
> +}
> +
> +/* { dg-final { scan-assembler "crset eq" } } */
> +/* { dg-final { scan-assembler "beqctrl-" } } */
> Index: gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-2.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-2.c     (nonexistent)
> +++ gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-2.c     (working copy)
> @@ -0,0 +1,33 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mno-speculate-indirect-jumps" } */
> +
> +/* Test for deliberate misprediction of computed goto.  */
> +
> +int bar (int);
> +int baz (int);
> +int spaz (int);
> +
> +int foo (int x)
> +{
> +  static void *labptr[] = { &&lab0, &&lab1, &&lab2 };
> +
> +  if (x < 0 || x > 2)
> +    return -1;
> +
> +  goto *labptr[x];
> +
> + lab0:
> +  return bar (x);
> +
> + lab1:
> +  return baz (x) + 1;
> +
> + lab2:
> +  return spaz (x) / 2;
> +}
> +
> +/* The following assumes CR7 as the first chosen volatile.  */
> +
> +/* { dg-final { scan-assembler "crset 30" } } */
> +/* { dg-final { scan-assembler "beqctr- 7" } } */
> +/* { dg-final { scan-assembler "b ." } } */
> Index: gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-3.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-3.c     (nonexistent)
> +++ gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-3.c     (working copy)
> @@ -0,0 +1,52 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mno-speculate-indirect-jumps" } */
> +
> +/* Test for deliberate misprediction of jump tables.  */
> +
> +void bar (void);
> +
> +int foo (int x)
> +{
> +  int a;
> +
> +  switch (x)
> +    {
> +    default:
> +      a = -1;
> +      break;
> +    case 0:
> +      a = x * x;
> +      break;
> +    case 1:
> +      a = x + 1;
> +      break;
> +    case 2:
> +      a = x + x;
> +      break;
> +    case 3:
> +      a = x << 3;
> +      break;
> +    case 4:
> +      a = x >> 1;
> +      break;
> +    case 5:
> +      a = x;
> +      break;
> +    case 6:
> +      a = 0;
> +      break;
> +    case 7:
> +      a = x * x + x;
> +      break;
> +    }
> +
> +  bar();
> +
> +  return a;
> +}
> +
> +/* The following assumes CR7 as the first chosen volatile.  */
> +
> +/* { dg-final { scan-assembler "crset 30" } } */
> +/* { dg-final { scan-assembler "beqctr- 7" } } */
> +/* { dg-final { scan-assembler "b ." } } */
> Index: gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-4.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-4.c     (nonexistent)
> +++ gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-4.c     (working copy)
> @@ -0,0 +1,25 @@
> +/* { dg-do run } */
> +/* { dg-additional-options "-mno-speculate-indirect-jumps" } */
> +
> +/* Test for deliberate misprediction of indirect calls for ELFv2.  */
> +
> +int (*f)();
> +
> +int __attribute__((noinline)) bar ()
> +{
> +  return (*f) ();
> +}
> +
> +int g ()
> +{
> +  return 26;
> +}
> +
> +int main ()
> +{
> +  f = &g;
> +  if (bar () != 26)
> +    __builtin_abort ();
> +
> +  return 0;
> +}
> Index: gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-5.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-5.c     (nonexistent)
> +++ gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-5.c     (working copy)
> @@ -0,0 +1,55 @@
> +/* { dg-do run } */
> +/* { dg-additional-options "-mno-speculate-indirect-jumps -Wno-pedantic" } */
> +
> +/* Test for deliberate misprediction of computed goto.  */
> +
> +int __attribute__((noinline)) bar (int i)
> +{
> +  return 1960 + i;
> +}
> +
> +int __attribute__((noinline)) baz (int i)
> +{
> +  return i * i;
> +}
> +
> +int __attribute__((noinline)) spaz (int i)
> +{
> +  return i + 1;
> +}
> +
> +int foo (int x)
> +{
> +  static void *labptr[] = { &&lab0, &&lab1, &&lab2 };
> +
> +  if (x < 0 || x > 2)
> +    return -1;
> +
> +  goto *labptr[x];
> +
> + lab0:
> +  return bar (x);
> +
> + lab1:
> +  return baz (x) + 1;
> +
> + lab2:
> +  return spaz (x) / 2;
> +}
> +
> +int main ()
> +{
> +  if (foo (0) != 1960)
> +    __builtin_abort ();
> +
> +  if (foo (1) != 2)
> +    __builtin_abort ();
> +
> +  if (foo (2) != 1)
> +    __builtin_abort ();
> +
> +  if (foo (3) != -1)
> +    __builtin_abort ();
> +
> +  return 0;
> +}
> Index: gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-6.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-6.c     (nonexistent)
> +++ gcc/testsuite/gcc.target/powerpc/safe-indirect-jump-6.c     (working copy)
> @@ -0,0 +1,80 @@
> +/* { dg-do run } */
> +/* { dg-additional-options "-mno-speculate-indirect-jumps" } */
> +
> +/* Test for deliberate misprediction of jump tables.  */
> +
> +void __attribute__((noinline)) bar ()
> +{
> +}
> +
> +int foo (int x)
> +{
> +  int a;
> +
> +  switch (x)
> +    {
> +    default:
> +      a = -1;
> +      break;
> +    case 0:
> +      a = x * x + 3;
> +      break;
> +    case 1:
> +      a = x + 1;
> +      break;
> +    case 2:
> +      a = x + x;
> +      break;
> +    case 3:
> +      a = x << 3;
> +      break;
> +    case 4:
> +      a = x >> 1;
> +      break;
> +    case 5:
> +      a = x;
> +      break;
> +    case 6:
> +      a = 0;
> +      break;
> +    case 7:
> +      a = x * x + x;
> +      break;
> +    }
> +
> +  bar();
> +
> +  return a;
> +}
> +
> +int main ()
> +{
> +  if (foo (0) != 3)
> +    __builtin_abort ();
> +
> +  if (foo (1) != 2)
> +    __builtin_abort ();
> +
> +  if (foo (2) != 4)
> +    __builtin_abort ();
> +
> +  if (foo (3) != 24)
> +    __builtin_abort ();
> +
> +  if (foo (4) != 2)
> +    __builtin_abort ();
> +
> +  if (foo (5) != 5)
> +    __builtin_abort ();
> +
> +  if (foo (6) != 0)
> +    __builtin_abort ();
> +
> +  if (foo (7) != 56)
> +    __builtin_abort ();
> +
> +  if (foo (8) != -1)
> +    __builtin_abort ();
> +
> +  return 0;
> +}
>

Hi!

On Tue, Jan 16, 2018 at 09:29:13AM +0100, Richard Biener wrote:
> Did you consider simply removing the tablejump/casesi support so
> expansion always
> expands to a balanced tree?  At least if we have any knobs to tune we
> should probably
> tweak them away from the indirect jump using variants with
> -mno-speculate-indirect-jumps,
> right?

We can generate indirect jumps for other situations so this patch will
still be needed.

> Performance optimization, so shouldn't block this patch - I just
> thought I should probably
> mention this.

Yeah let's get this done first :-)


Segher

Hi!

On Mon, Jan 15, 2018 at 05:09:06PM -0600, Bill Schmidt wrote:
> @@ -12933,9 +12974,27 @@
>    ""
>  {
>    if (TARGET_32BIT)
> -    emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
> +    {
> +      if (rs6000_speculate_indirect_jumps)
> +	emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
> +      else
> +	{
> +	  rtx ccreg = gen_reg_rtx (CCmode);
> +	  rtx jump = gen_tablejumpsi_nospec (operands[0], operands[1], ccreg);
> +	  emit_jump_insn (jump);
> +	}
> +    }
>    else
> -    emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
> +    {
> +      if (rs6000_speculate_indirect_jumps)
> +	emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
> +      else
> +	{
> +	  rtx ccreg = gen_reg_rtx (CCmode);
> +	  rtx jump = gen_tablejumpdi_nospec (operands[0], operands[1], ccreg);
> +	  emit_jump_insn (jump);
> +	}
> +    }
>    DONE;
>  })

This is easier to read if you swap the "if"s (put the
rs6000_speculate_indirect_jumps test on the outside).

Okay for trunk with or without such a change.  Also okay for the branches
after some testing (esp. on other ABIs, it is easy to break those together
with -mno-speculate-indirect-branches since no one sane would use that
combo on purpose).

Thanks!


Segher

On Jan 16, 2018, at 6:13 AM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> Hi!
> 
> On Tue, Jan 16, 2018 at 09:29:13AM +0100, Richard Biener wrote:
>> Did you consider simply removing the tablejump/casesi support so
>> expansion always
>> expands to a balanced tree?  At least if we have any knobs to tune we
>> should probably
>> tweak them away from the indirect jump using variants with
>> -mno-speculate-indirect-jumps,
>> right?
> 
> We can generate indirect jumps for other situations so this patch will
> still be needed.

Also, I'm not convinced that a balanced tree for a large jump table
is a slam dunk better performer than this (adding hundreds of poorly
predictable branches that can clog up hardware predictors for, say,
an interpreter loop).  I'd want to do some performance testing to look
for crossover points (as you say, tuning knobs).  But for smaller tables
this is a good idea.

Thanks,
Bill
> 
>> Performance optimization, so shouldn't block this patch - I just
>> thought I should probably
>> mention this.
> 
> Yeah let's get this done first :-)
> 
> 
> Segher
>

On 01/15/2018 04:09 PM, Bill Schmidt wrote:

> [gcc]
> 
> 2018-01-15  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	* config/rs6000/rs6000.c (rs6000_opt_vars): Add entry for
> 	-mspeculate-indirect-jumps.
> 	* config/rs6000/rs6000.md (*call_indirect_elfv2<mode>): Disable
> 	for -mno-speculate-indirect-jumps.
> 	(*call_indirect_elfv2<mode>_nospec): New define_insn.
> 	(*call_value_indirect_elfv2<mode>): Disable for
> 	-mno-speculate-indirect-jumps.
> 	(*call_value_indirect_elfv2<mode>_nospec): New define_insn.
> 	(indirect_jump): Emit different RTL for
> 	-mno-speculate-indirect-jumps.
> 	(*indirect_jump<mode>): Disable for
> 	-mno-speculate-indirect-jumps.
> 	(*indirect_jump<mode>_nospec): New define_insn.
> 	(tablejump): Emit different RTL for
> 	-mno-speculate-indirect-jumps.
> 	(tablejumpsi): Disable for -mno-speculate-indirect-jumps.
> 	(tablejumpsi_nospec): New define_expand.
> 	(tablejumpdi): Disable for -mno-speculate-indirect-jumps.
> 	(tablejumpdi_nospec): New define_expand.
> 	(*tablejump<mode>_internal1): Disable for
> 	-mno-speculate-indirect-jumps.
> 	(*tablejump<mode>_internal1_nospec): New define_insn.
> 	* config/rs6000/rs6000.opt (mspeculate-indirect-jumps): New
> 	option.

I see no documentation for the new option here....  :-(

-Sandra

On Fri, Jan 19, 2018 at 10:01:50AM -0700, Sandra Loosemore wrote:
> 
> I see no documentation for the new option here....  :-(

+;; -mno-speculate-indirect-jumps adds deliberate misprediction to indirect
+;; branches via the CTR.
+mspeculate-indirect-jumps
+Target Undocumented Var(rs6000_speculate_indirect_jumps) Init(1) Save

It's undocumented on purpose.  I don't know if that is the best idea.


Segher