[v2,rs6000] Add -msafe-indirect-jumps option and implement safe bctr / bctrl

Hi,

[This patch supercedes and extends https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01135.html.
There was a small error in the assembly code produced by that patch (bad
memory on my account of how to spell "crset eq").  I've also increased the
function provided; see below.]

This patch adds a new option for the compiler to produce only "safe" indirect
jumps, in the sense that these jumps are deliberately mispredicted to inhibit
speculative execution.  For now, this option is undocumented; this may change
at some future date.  It is intended eventually for the linker to also honor
this flag when creating PLT stubs, for example.

In addition to the new option, I've included changes to indirect calls for
the ELFv2 ABI when the option is specified.  In place of bctrl, we generate
a "crset eq" followed by a beqctrl-.  Using the CR0.eq bit is safe since CR0
is volatile over the call.

I've also added code to replace uses of bctr when the new option is specified,
with the sequence

	crset 4x[CRb]+2
	beqctr- CRb
	b .

where CRb is an available condition register field.  This applies to all
subtargets, and in particular is not restricted to ELFv2.  The use cases
covered here are computed gotos and switch statements.

NOT yet covered by this patch: indirect calls for ELFv1.  That will come later.

Please let me know if there is a better way to represent the crset without
an unspec.  For the indirect jump, I don't see a way around it due to the
expected form of indirect jumps in cfganal.c.

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

Thanks,
Bill


[gcc]

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

	* config/rs6000/rs6000.c (rs6000_opt_vars): Add entry for
	-msafe-indirect-jumps.
	* config/rs6000/rs6000.md (UNSPEC_CRSET_EQ): New UNSPEC enum.
	(UNSPEC_COMP_GOTO_CR): Likewise.
	(*call_indirect_elfv2<mode>): Disable for -msafe-indirect-jumps.
	(*call_indirect_elfv2<mode>_safe): New define_insn.
	(*call_value_indirect_elfv2<mode>): Disable for
	-msafe-indirect-jumps.
	(*call_value_indirect_elfv2<mode>_safe): New define_insn.
	(indirect_jump): Emit different RTL for -msafe-indirect-jumps.
	(*indirect_jump<mode>): Disable for -msafe-indirect-jumps.
	(*indirect_jump<mode>_safe): New define_insn.
	(*set_cr_eq): New define_insn.
	(tablejump): Emit different RTL for -msafe-indirect-jumps.
	(tablejumpsi): Disable for -msafe-indirect-jumps.
	(tablejumpsi_safe): New define_expand.
	(tablejumpdi): Disable for -msafe-indirect-jumps.
	(tablejumpdi_safe): New define_expand.
	(*tablejump<mode>_internal1): Disable for -msafe-indirect-jumps.
	(*tablejump<mode>_internal1_safe): New define_insn.
	* config/rs6000/rs6000.opt (msafe-indirect-jumps): New option.

[gcc/testsuite]

2018-01-13  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.

On Sun, Jan 14, 2018 at 5:53 AM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> Hi,
>
> [This patch supercedes and extends https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01135.html.
> There was a small error in the assembly code produced by that patch (bad
> memory on my account of how to spell "crset eq").  I've also increased the
> function provided; see below.]
>
> This patch adds a new option for the compiler to produce only "safe" indirect
> jumps, in the sense that these jumps are deliberately mispredicted to inhibit
> speculative execution.  For now, this option is undocumented; this may change
> at some future date.  It is intended eventually for the linker to also honor
> this flag when creating PLT stubs, for example.
>
> In addition to the new option, I've included changes to indirect calls for
> the ELFv2 ABI when the option is specified.  In place of bctrl, we generate
> a "crset eq" followed by a beqctrl-.  Using the CR0.eq bit is safe since CR0
> is volatile over the call.
>
> I've also added code to replace uses of bctr when the new option is specified,
> with the sequence
>
>         crset 4x[CRb]+2
>         beqctr- CRb
>         b .
>
> where CRb is an available condition register field.  This applies to all
> subtargets, and in particular is not restricted to ELFv2.  The use cases
> covered here are computed gotos and switch statements.
>
> NOT yet covered by this patch: indirect calls for ELFv1.  That will come later.
>
> Please let me know if there is a better way to represent the crset without
> an unspec.  For the indirect jump, I don't see a way around it due to the
> expected form of indirect jumps in cfganal.c.
>
> Bootstrapped and tested on powerpc64-linux-gnu and powerpc64le-linux-gnu with
> no regressions.  Is this okay for trunk?

As this sounds Spectre related feel free to backport this to the GCC 7 branch
as well (even if you don't hit the Wednesday deadline for RC1 of GCC 7.3).

Thanks,
Richard.

> Thanks,
> Bill
>
>
> [gcc]
>
> 2018-01-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
>
>         * config/rs6000/rs6000.c (rs6000_opt_vars): Add entry for
>         -msafe-indirect-jumps.
>         * config/rs6000/rs6000.md (UNSPEC_CRSET_EQ): New UNSPEC enum.
>         (UNSPEC_COMP_GOTO_CR): Likewise.
>         (*call_indirect_elfv2<mode>): Disable for -msafe-indirect-jumps.
>         (*call_indirect_elfv2<mode>_safe): New define_insn.
>         (*call_value_indirect_elfv2<mode>): Disable for
>         -msafe-indirect-jumps.
>         (*call_value_indirect_elfv2<mode>_safe): New define_insn.
>         (indirect_jump): Emit different RTL for -msafe-indirect-jumps.
>         (*indirect_jump<mode>): Disable for -msafe-indirect-jumps.
>         (*indirect_jump<mode>_safe): New define_insn.
>         (*set_cr_eq): New define_insn.
>         (tablejump): Emit different RTL for -msafe-indirect-jumps.
>         (tablejumpsi): Disable for -msafe-indirect-jumps.
>         (tablejumpsi_safe): New define_expand.
>         (tablejumpdi): Disable for -msafe-indirect-jumps.
>         (tablejumpdi_safe): New define_expand.
>         (*tablejump<mode>_internal1): Disable for -msafe-indirect-jumps.
>         (*tablejump<mode>_internal1_safe): New define_insn.
>         * config/rs6000/rs6000.opt (msafe-indirect-jumps): New option.
>
> [gcc/testsuite]
>
> 2018-01-13  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.
>
>
> 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), },
> +  { "safe-indirect-jumps",
> +    offsetof (struct gcc_options, x_rs6000_safe_indirect_jumps),
> +    offsetof (struct cl_target_option, x_rs6000_safe_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)
> @@ -150,6 +150,8 @@
>     UNSPEC_SIGNBIT
>     UNSPEC_SF_FROM_SI
>     UNSPEC_SI_FROM_SF
> +   UNSPEC_CRSET_EQ
> +   UNSPEC_COMP_GOTO_CR
>    ])
>
>  ;;
> @@ -11222,11 +11224,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_safe_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>_safe"
> +  [(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_safe_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 +11246,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_safe_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>_safe"
> +  [(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_safe_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 +12941,50 @@
>    [(set_attr "type" "jmpreg")])
>
>  (define_expand "indirect_jump"
> -  [(set (pc) (match_operand 0 "register_operand"))])
> +  [(set (pc) (match_operand 0 "register_operand"))]
> + ""
> +{
> +  /* We need to reserve a CR when forcing a mispredicted jump.  */
> +  if (rs6000_safe_indirect_jumps) {
> +    rtx ccreg = gen_reg_rtx (CCmode);
> +    emit_insn (gen_rtx_SET (ccreg,
> +                           gen_rtx_UNSPEC (CCmode,
> +                                           gen_rtvec (1, const0_rtx),
> +                                           UNSPEC_CRSET_EQ)));
> +    rtvec v = rtvec_alloc (2);
> +    RTVEC_ELT (v, 0) = operands[0];
> +    RTVEC_ELT (v, 1) = ccreg;
> +    emit_jump_insn (gen_rtx_SET (pc_rtx,
> +                                gen_rtx_UNSPEC (Pmode, v,
> +                                                UNSPEC_COMP_GOTO_CR)));
> +    DONE;
> +  }
> +})
>
>  (define_insn "*indirect_jump<mode>"
>    [(set (pc)
>         (match_operand:P 0 "register_operand" "c,*l"))]
> -  ""
> +  "!rs6000_safe_indirect_jumps"
>    "b%T0"
>    [(set_attr "type" "jmpreg")])
>
> +(define_insn "*indirect_jump<mode>_safe"
> +  [(set (pc)
> +       (unspec:P [(match_operand:P 0 "register_operand" "c,*l")
> +                  (match_operand:CC 1 "cc_reg_operand" "y,y")]
> +                  UNSPEC_COMP_GOTO_CR))]
> +  "rs6000_safe_indirect_jumps"
> +  "beq%T0- %1\;b ."
> +  [(set_attr "type" "jmpreg")
> +   (set_attr "length" "8")])
> +
> +(define_insn "*set_cr_eq"
> +  [(set (match_operand:CC 0 "cc_reg_operand" "=y")
> +       (unspec:CC [(const_int 0)] UNSPEC_CRSET_EQ))]
> +  "rs6000_safe_indirect_jumps"
> +  "crset %E0"
> +  [(set_attr "type" "cr_logical")])
> +
>  ;; Table jump for switch statements:
>  (define_expand "tablejump"
>    [(use (match_operand 0))
> @@ -12933,9 +12992,19 @@
>    ""
>  {
>    if (TARGET_32BIT)
> -    emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
> +    {
> +      if (rs6000_safe_indirect_jumps)
> +       emit_jump_insn (gen_tablejumpsi_safe (operands[0], operands[1]));
> +      else
> +       emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
> +    }
>    else
> -    emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
> +    {
> +      if (rs6000_safe_indirect_jumps)
> +       emit_jump_insn (gen_tablejumpdi_safe (operands[0], operands[1]));
> +      else
> +       emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
> +    }
>    DONE;
>  })
>
> @@ -12946,7 +13015,7 @@
>     (parallel [(set (pc)
>                    (match_dup 3))
>               (use (label_ref (match_operand 1)))])]
> -  "TARGET_32BIT"
> +  "TARGET_32BIT && !rs6000_safe_indirect_jumps"
>  {
>    operands[0] = force_reg (SImode, operands[0]);
>    operands[2] = force_reg (SImode, gen_rtx_LABEL_REF (SImode, operands[1]));
> @@ -12953,6 +13022,23 @@
>    operands[3] = gen_reg_rtx (SImode);
>  })
>
> +(define_expand "tablejumpsi_safe"
> +  [(set (match_dup 3)
> +       (plus:SI (match_operand:SI 0)
> +                (match_dup 2)))
> +   (set (match_dup 4) (unspec:CC [(const_int 0)] UNSPEC_CRSET_EQ))
> +   (parallel [(set (pc)
> +                  (match_dup 3))
> +             (use (label_ref (match_operand 1)))
> +             (use (match_dup 4))])]
> +  "TARGET_32BIT && rs6000_safe_indirect_jumps"
> +{
> +  operands[0] = force_reg (SImode, operands[0]);
> +  operands[2] = force_reg (SImode, gen_rtx_LABEL_REF (SImode, operands[1]));
> +  operands[3] = gen_reg_rtx (SImode);
> +  operands[4] = gen_reg_rtx (CCmode);
> +})
> +
>  (define_expand "tablejumpdi"
>    [(set (match_dup 4)
>          (sign_extend:DI (match_operand:SI 0 "lwa_operand")))
> @@ -12962,7 +13048,7 @@
>     (parallel [(set (pc)
>                    (match_dup 3))
>               (use (label_ref (match_operand 1)))])]
> -  "TARGET_64BIT"
> +  "TARGET_64BIT && !rs6000_safe_indirect_jumps"
>  {
>    operands[2] = force_reg (DImode, gen_rtx_LABEL_REF (DImode, operands[1]));
>    operands[3] = gen_reg_rtx (DImode);
> @@ -12969,14 +13055,43 @@
>    operands[4] = gen_reg_rtx (DImode);
>  })
>
> +(define_expand "tablejumpdi_safe"
> +  [(set (match_dup 4)
> +        (sign_extend:DI (match_operand:SI 0 "lwa_operand")))
> +   (set (match_dup 5) (unspec:CC [(const_int 0)] UNSPEC_CRSET_EQ))
> +   (set (match_dup 3)
> +       (plus:DI (match_dup 4)
> +                (match_dup 2)))
> +   (parallel [(set (pc)
> +                  (match_dup 3))
> +             (use (label_ref (match_operand 1)))
> +             (use (match_dup 5))])]
> +  "TARGET_64BIT && rs6000_safe_indirect_jumps"
> +{
> +  operands[2] = force_reg (DImode, gen_rtx_LABEL_REF (DImode, operands[1]));
> +  operands[3] = gen_reg_rtx (DImode);
> +  operands[4] = gen_reg_rtx (DImode);
> +  operands[5] = gen_reg_rtx (CCmode);
> +})
> +
>  (define_insn "*tablejump<mode>_internal1"
>    [(set (pc)
>         (match_operand:P 0 "register_operand" "c,*l"))
>     (use (label_ref (match_operand 1)))]
> -  ""
> +  "!rs6000_safe_indirect_jumps"
>    "b%T0"
>    [(set_attr "type" "jmpreg")])
>
> +(define_insn "*tablejump<mode>_internal1_safe"
> +  [(set (pc)
> +       (match_operand:P 0 "register_operand" "c,*l"))
> +   (use (label_ref (match_operand 1)))
> +   (use (match_operand:CC 2 "cc_reg_operand" "y,y"))]
> +  "rs6000_safe_indirect_jumps"
> +  "beq%T0- %2\;b ."
> +  [(set_attr "type" "jmpreg")
> +   (set_attr "length" "8")])
> +
>  (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
> +
> +;; -msafe-indirect-jumps adds deliberate misprediction to indirect
> +;; branches via the CTR.
> +msafe-indirect-jumps
> +Target Undocumented Var(rs6000_safe_indirect_jumps) Init(0) 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-options "-msafe-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 { target { powerpc*-*-* } } } */
> +/* { dg-options "-msafe-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 { target { powerpc*-*-* } } } */
> +/* { dg-options "-msafe-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 ." } } */
>

On Jan 15, 2018, at 3:46 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Sun, Jan 14, 2018 at 5:53 AM, Bill Schmidt
> <wschmidt@linux.vnet.ibm.com> wrote:
>> Hi,
>> 
>> [This patch supercedes and extends https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01135.html.
>> There was a small error in the assembly code produced by that patch (bad
>> memory on my account of how to spell "crset eq").  I've also increased the
>> function provided; see below.]
>> 
>> This patch adds a new option for the compiler to produce only "safe" indirect
>> jumps, in the sense that these jumps are deliberately mispredicted to inhibit
>> speculative execution.  For now, this option is undocumented; this may change
>> at some future date.  It is intended eventually for the linker to also honor
>> this flag when creating PLT stubs, for example.
>> 
>> In addition to the new option, I've included changes to indirect calls for
>> the ELFv2 ABI when the option is specified.  In place of bctrl, we generate
>> a "crset eq" followed by a beqctrl-.  Using the CR0.eq bit is safe since CR0
>> is volatile over the call.
>> 
>> I've also added code to replace uses of bctr when the new option is specified,
>> with the sequence
>> 
>>        crset 4x[CRb]+2
>>        beqctr- CRb
>>        b .
>> 
>> where CRb is an available condition register field.  This applies to all
>> subtargets, and in particular is not restricted to ELFv2.  The use cases
>> covered here are computed gotos and switch statements.
>> 
>> NOT yet covered by this patch: indirect calls for ELFv1.  That will come later.
>> 
>> Please let me know if there is a better way to represent the crset without
>> an unspec.  For the indirect jump, I don't see a way around it due to the
>> expected form of indirect jumps in cfganal.c.
>> 
>> Bootstrapped and tested on powerpc64-linux-gnu and powerpc64le-linux-gnu with
>> no regressions.  Is this okay for trunk?
> 
> As this sounds Spectre related feel free to backport this to the GCC 7 branch
> as well (even if you don't hit the Wednesday deadline for RC1 of GCC 7.3).

Thanks, Richard -- I hadn't seen this deadline announced, but I will do my best
to get this completed/committed by then.  Thanks for the heads-up!

Bill

> 
> Thanks,
> Richard.
> 
>> Thanks,
>> Bill
>> 
>> 
>> [gcc]
>> 
>> 2018-01-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
>> 
>>        * config/rs6000/rs6000.c (rs6000_opt_vars): Add entry for
>>        -msafe-indirect-jumps.
>>        * config/rs6000/rs6000.md (UNSPEC_CRSET_EQ): New UNSPEC enum.
>>        (UNSPEC_COMP_GOTO_CR): Likewise.
>>        (*call_indirect_elfv2<mode>): Disable for -msafe-indirect-jumps.
>>        (*call_indirect_elfv2<mode>_safe): New define_insn.
>>        (*call_value_indirect_elfv2<mode>): Disable for
>>        -msafe-indirect-jumps.
>>        (*call_value_indirect_elfv2<mode>_safe): New define_insn.
>>        (indirect_jump): Emit different RTL for -msafe-indirect-jumps.
>>        (*indirect_jump<mode>): Disable for -msafe-indirect-jumps.
>>        (*indirect_jump<mode>_safe): New define_insn.
>>        (*set_cr_eq): New define_insn.
>>        (tablejump): Emit different RTL for -msafe-indirect-jumps.
>>        (tablejumpsi): Disable for -msafe-indirect-jumps.
>>        (tablejumpsi_safe): New define_expand.
>>        (tablejumpdi): Disable for -msafe-indirect-jumps.
>>        (tablejumpdi_safe): New define_expand.
>>        (*tablejump<mode>_internal1): Disable for -msafe-indirect-jumps.
>>        (*tablejump<mode>_internal1_safe): New define_insn.
>>        * config/rs6000/rs6000.opt (msafe-indirect-jumps): New option.
>> 
>> [gcc/testsuite]
>> 
>> 2018-01-13  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.
>> 
>> 
>> 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), },
>> +  { "safe-indirect-jumps",
>> +    offsetof (struct gcc_options, x_rs6000_safe_indirect_jumps),
>> +    offsetof (struct cl_target_option, x_rs6000_safe_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)
>> @@ -150,6 +150,8 @@
>>    UNSPEC_SIGNBIT
>>    UNSPEC_SF_FROM_SI
>>    UNSPEC_SI_FROM_SF
>> +   UNSPEC_CRSET_EQ
>> +   UNSPEC_COMP_GOTO_CR
>>   ])
>> 
>> ;;
>> @@ -11222,11 +11224,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_safe_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>_safe"
>> +  [(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_safe_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 +11246,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_safe_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>_safe"
>> +  [(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_safe_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 +12941,50 @@
>>   [(set_attr "type" "jmpreg")])
>> 
>> (define_expand "indirect_jump"
>> -  [(set (pc) (match_operand 0 "register_operand"))])
>> +  [(set (pc) (match_operand 0 "register_operand"))]
>> + ""
>> +{
>> +  /* We need to reserve a CR when forcing a mispredicted jump.  */
>> +  if (rs6000_safe_indirect_jumps) {
>> +    rtx ccreg = gen_reg_rtx (CCmode);
>> +    emit_insn (gen_rtx_SET (ccreg,
>> +                           gen_rtx_UNSPEC (CCmode,
>> +                                           gen_rtvec (1, const0_rtx),
>> +                                           UNSPEC_CRSET_EQ)));
>> +    rtvec v = rtvec_alloc (2);
>> +    RTVEC_ELT (v, 0) = operands[0];
>> +    RTVEC_ELT (v, 1) = ccreg;
>> +    emit_jump_insn (gen_rtx_SET (pc_rtx,
>> +                                gen_rtx_UNSPEC (Pmode, v,
>> +                                                UNSPEC_COMP_GOTO_CR)));
>> +    DONE;
>> +  }
>> +})
>> 
>> (define_insn "*indirect_jump<mode>"
>>   [(set (pc)
>>        (match_operand:P 0 "register_operand" "c,*l"))]
>> -  ""
>> +  "!rs6000_safe_indirect_jumps"
>>   "b%T0"
>>   [(set_attr "type" "jmpreg")])
>> 
>> +(define_insn "*indirect_jump<mode>_safe"
>> +  [(set (pc)
>> +       (unspec:P [(match_operand:P 0 "register_operand" "c,*l")
>> +                  (match_operand:CC 1 "cc_reg_operand" "y,y")]
>> +                  UNSPEC_COMP_GOTO_CR))]
>> +  "rs6000_safe_indirect_jumps"
>> +  "beq%T0- %1\;b ."
>> +  [(set_attr "type" "jmpreg")
>> +   (set_attr "length" "8")])
>> +
>> +(define_insn "*set_cr_eq"
>> +  [(set (match_operand:CC 0 "cc_reg_operand" "=y")
>> +       (unspec:CC [(const_int 0)] UNSPEC_CRSET_EQ))]
>> +  "rs6000_safe_indirect_jumps"
>> +  "crset %E0"
>> +  [(set_attr "type" "cr_logical")])
>> +
>> ;; Table jump for switch statements:
>> (define_expand "tablejump"
>>   [(use (match_operand 0))
>> @@ -12933,9 +12992,19 @@
>>   ""
>> {
>>   if (TARGET_32BIT)
>> -    emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
>> +    {
>> +      if (rs6000_safe_indirect_jumps)
>> +       emit_jump_insn (gen_tablejumpsi_safe (operands[0], operands[1]));
>> +      else
>> +       emit_jump_insn (gen_tablejumpsi (operands[0], operands[1]));
>> +    }
>>   else
>> -    emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
>> +    {
>> +      if (rs6000_safe_indirect_jumps)
>> +       emit_jump_insn (gen_tablejumpdi_safe (operands[0], operands[1]));
>> +      else
>> +       emit_jump_insn (gen_tablejumpdi (operands[0], operands[1]));
>> +    }
>>   DONE;
>> })
>> 
>> @@ -12946,7 +13015,7 @@
>>    (parallel [(set (pc)
>>                   (match_dup 3))
>>              (use (label_ref (match_operand 1)))])]
>> -  "TARGET_32BIT"
>> +  "TARGET_32BIT && !rs6000_safe_indirect_jumps"
>> {
>>   operands[0] = force_reg (SImode, operands[0]);
>>   operands[2] = force_reg (SImode, gen_rtx_LABEL_REF (SImode, operands[1]));
>> @@ -12953,6 +13022,23 @@
>>   operands[3] = gen_reg_rtx (SImode);
>> })
>> 
>> +(define_expand "tablejumpsi_safe"
>> +  [(set (match_dup 3)
>> +       (plus:SI (match_operand:SI 0)
>> +                (match_dup 2)))
>> +   (set (match_dup 4) (unspec:CC [(const_int 0)] UNSPEC_CRSET_EQ))
>> +   (parallel [(set (pc)
>> +                  (match_dup 3))
>> +             (use (label_ref (match_operand 1)))
>> +             (use (match_dup 4))])]
>> +  "TARGET_32BIT && rs6000_safe_indirect_jumps"
>> +{
>> +  operands[0] = force_reg (SImode, operands[0]);
>> +  operands[2] = force_reg (SImode, gen_rtx_LABEL_REF (SImode, operands[1]));
>> +  operands[3] = gen_reg_rtx (SImode);
>> +  operands[4] = gen_reg_rtx (CCmode);
>> +})
>> +
>> (define_expand "tablejumpdi"
>>   [(set (match_dup 4)
>>         (sign_extend:DI (match_operand:SI 0 "lwa_operand")))
>> @@ -12962,7 +13048,7 @@
>>    (parallel [(set (pc)
>>                   (match_dup 3))
>>              (use (label_ref (match_operand 1)))])]
>> -  "TARGET_64BIT"
>> +  "TARGET_64BIT && !rs6000_safe_indirect_jumps"
>> {
>>   operands[2] = force_reg (DImode, gen_rtx_LABEL_REF (DImode, operands[1]));
>>   operands[3] = gen_reg_rtx (DImode);
>> @@ -12969,14 +13055,43 @@
>>   operands[4] = gen_reg_rtx (DImode);
>> })
>> 
>> +(define_expand "tablejumpdi_safe"
>> +  [(set (match_dup 4)
>> +        (sign_extend:DI (match_operand:SI 0 "lwa_operand")))
>> +   (set (match_dup 5) (unspec:CC [(const_int 0)] UNSPEC_CRSET_EQ))
>> +   (set (match_dup 3)
>> +       (plus:DI (match_dup 4)
>> +                (match_dup 2)))
>> +   (parallel [(set (pc)
>> +                  (match_dup 3))
>> +             (use (label_ref (match_operand 1)))
>> +             (use (match_dup 5))])]
>> +  "TARGET_64BIT && rs6000_safe_indirect_jumps"
>> +{
>> +  operands[2] = force_reg (DImode, gen_rtx_LABEL_REF (DImode, operands[1]));
>> +  operands[3] = gen_reg_rtx (DImode);
>> +  operands[4] = gen_reg_rtx (DImode);
>> +  operands[5] = gen_reg_rtx (CCmode);
>> +})
>> +
>> (define_insn "*tablejump<mode>_internal1"
>>   [(set (pc)
>>        (match_operand:P 0 "register_operand" "c,*l"))
>>    (use (label_ref (match_operand 1)))]
>> -  ""
>> +  "!rs6000_safe_indirect_jumps"
>>   "b%T0"
>>   [(set_attr "type" "jmpreg")])
>> 
>> +(define_insn "*tablejump<mode>_internal1_safe"
>> +  [(set (pc)
>> +       (match_operand:P 0 "register_operand" "c,*l"))
>> +   (use (label_ref (match_operand 1)))
>> +   (use (match_operand:CC 2 "cc_reg_operand" "y,y"))]
>> +  "rs6000_safe_indirect_jumps"
>> +  "beq%T0- %2\;b ."
>> +  [(set_attr "type" "jmpreg")
>> +   (set_attr "length" "8")])
>> +
>> (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
>> +
>> +;; -msafe-indirect-jumps adds deliberate misprediction to indirect
>> +;; branches via the CTR.
>> +msafe-indirect-jumps
>> +Target Undocumented Var(rs6000_safe_indirect_jumps) Init(0) 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-options "-msafe-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 { target { powerpc*-*-* } } } */
>> +/* { dg-options "-msafe-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 { target { powerpc*-*-* } } } */
>> +/* { dg-options "-msafe-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 ." } } */

Hi!

On Sat, Jan 13, 2018 at 10:53:57PM -0600, Bill Schmidt wrote:
> This patch adds a new option for the compiler to produce only "safe" indirect
> jumps, in the sense that these jumps are deliberately mispredicted to inhibit
> speculative execution.  For now, this option is undocumented; this may change
> at some future date.  It is intended eventually for the linker to also honor
> this flag when creating PLT stubs, for example.

I think we settled on calling the option -mmispredict-indirect-jumps;
please let me know if you still agree with that.  Or have thought of a
better name :-)

> In addition to the new option, I've included changes to indirect calls for
> the ELFv2 ABI when the option is specified.  In place of bctrl, we generate
> a "crset eq" followed by a beqctrl-.  Using the CR0.eq bit is safe since CR0
> is volatile over the call.

And CR0 is unused by the call; compare to CR1 (on older ABIs) for example.

> I've also added code to replace uses of bctr when the new option is specified,
> with the sequence
> 
> 	crset 4x[CRb]+2
> 	beqctr- CRb
> 	b .
> 
> where CRb is an available condition register field.  This applies to all
> subtargets, and in particular is not restricted to ELFv2.  The use cases
> covered here are computed gotos and switch statements.
> 
> NOT yet covered by this patch: indirect calls for ELFv1.  That will come later.

Would be nice to have it for all ABIs, even.

> Please let me know if there is a better way to represent the crset without
> an unspec.

See the various patterns using cr%q.  I'm not sure they can generate creqv
(i.e. crset) currently, but that could be added (like crnot is there already,
for example).  If you don't use unspec (or maybe unspec_volatile) it can be
optimised away though.

Maybe it is best not to put the crset into its own insn, just make it part
of the bigger pattern, with an appropriate clobber?

> For the indirect jump, I don't see a way around it due to the
> expected form of indirect jumps in cfganal.c.

I'm not sure what you are getting at here, could you explain a bit?

>  (define_expand "indirect_jump"
> -  [(set (pc) (match_operand 0 "register_operand"))])
> +  [(set (pc) (match_operand 0 "register_operand"))]
> + ""
> +{
> +  /* We need to reserve a CR when forcing a mispredicted jump.  */
> +  if (rs6000_safe_indirect_jumps) {
> +    rtx ccreg = gen_reg_rtx (CCmode);
> +    emit_insn (gen_rtx_SET (ccreg,
> +			    gen_rtx_UNSPEC (CCmode,
> +					    gen_rtvec (1, const0_rtx),
> +					    UNSPEC_CRSET_EQ)));
> +    rtvec v = rtvec_alloc (2);
> +    RTVEC_ELT (v, 0) = operands[0];
> +    RTVEC_ELT (v, 1) = ccreg;
> +    emit_jump_insn (gen_rtx_SET (pc_rtx,
> +				 gen_rtx_UNSPEC (Pmode, v,
> +						 UNSPEC_COMP_GOTO_CR)));
> +    DONE;
> +  }
> +})
>  
>  (define_insn "*indirect_jump<mode>"
>    [(set (pc)
>  	(match_operand:P 0 "register_operand" "c,*l"))]
> -  ""
> +  "!rs6000_safe_indirect_jumps"
>    "b%T0"
>    [(set_attr "type" "jmpreg")])
>  
> +(define_insn "*indirect_jump<mode>_safe"
> +  [(set (pc)
> +	(unspec:P [(match_operand:P 0 "register_operand" "c,*l")
> +		   (match_operand:CC 1 "cc_reg_operand" "y,y")]
> +		   UNSPEC_COMP_GOTO_CR))]
> +  "rs6000_safe_indirect_jumps"
> +  "beq%T0- %1\;b ."
> +  [(set_attr "type" "jmpreg")
> +   (set_attr "length" "8")])
> +
> +(define_insn "*set_cr_eq"
> +  [(set (match_operand:CC 0 "cc_reg_operand" "=y")
> +	(unspec:CC [(const_int 0)] UNSPEC_CRSET_EQ))]
> +  "rs6000_safe_indirect_jumps"
> +  "crset %E0"
> +  [(set_attr "type" "cr_logical")])

So merge this latter insn into the previous, making the CC a clobber?
Like (not tested):

+(define_insn "indirect_jump<mode>_mispredict"
+  [(set (pc)
+	(match_operand:P 0 "register_operand" "c,*l")
+   (clobber (match_operand:CC 1 "cc_reg_operand" "y,y"))]
+  "rs6000_safe_indirect_jumps"
+  "crset %E1\;beq%T0- %1\;b ."
+  [(set_attr "type" "jmpreg")
+   (set_attr "length" "12")])

and then change the indirect_jump pattern to simply select between the normal
and mispredict patterns?

> +(define_expand "tablejumpsi_safe"

And then similar for tablejump.


Segher

Hi Segher,

Thanks for the quick review!

> On Jan 15, 2018, at 10:38 AM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> Hi!
> 
> On Sat, Jan 13, 2018 at 10:53:57PM -0600, Bill Schmidt wrote:
>> This patch adds a new option for the compiler to produce only "safe" indirect
>> jumps, in the sense that these jumps are deliberately mispredicted to inhibit
>> speculative execution.  For now, this option is undocumented; this may change
>> at some future date.  It is intended eventually for the linker to also honor
>> this flag when creating PLT stubs, for example.
> 
> I think we settled on calling the option -mmispredict-indirect-jumps;
> please let me know if you still agree with that.  Or have thought of a
> better name :-)

Looks like we are now looking at -m[no-]speculate-indirect-jumps with
default to true.  LLVM folks are in agreement too.
> 
>> In addition to the new option, I've included changes to indirect calls for
>> the ELFv2 ABI when the option is specified.  In place of bctrl, we generate
>> a "crset eq" followed by a beqctrl-.  Using the CR0.eq bit is safe since CR0
>> is volatile over the call.
> 
> And CR0 is unused by the call; compare to CR1 (on older ABIs) for example.
> 
>> I've also added code to replace uses of bctr when the new option is specified,
>> with the sequence
>> 
>> 	crset 4x[CRb]+2
>> 	beqctr- CRb
>> 	b .
>> 
>> where CRb is an available condition register field.  This applies to all
>> subtargets, and in particular is not restricted to ELFv2.  The use cases
>> covered here are computed gotos and switch statements.
>> 
>> NOT yet covered by this patch: indirect calls for ELFv1.  That will come later.
> 
> Would be nice to have it for all ABIs, even.

Yeah, that's the plan.  Everything that uses bctr[l].  I used too loose of language.
> 
>> Please let me know if there is a better way to represent the crset without
>> an unspec.
> 
> See the various patterns using cr%q.  I'm not sure they can generate creqv
> (i.e. crset) currently, but that could be added (like crnot is there already,
> for example).  If you don't use unspec (or maybe unspec_volatile) it can be
> optimised away though.
> 
> Maybe it is best not to put the crset into its own insn, just make it part
> of the bigger pattern, with an appropriate clobber?
> 
>> For the indirect jump, I don't see a way around it due to the
>> expected form of indirect jumps in cfganal.c.
> 
> I'm not sure what you are getting at here, could you explain a bit?

An indirect jump is expected to be of the form (set (pc) (...other stuff));
otherwise it might get missed and blocks that are only reachable from
an indirect jump can get deleted.  I found this out when I tried to do
something involving a parallel that was not very bright; that doesn't
actually prevent anything if you are doing things right.  And the
clobber solution you suggest should be just fine.

tldr:  Ignore my lunatic ravings. ;-)

Thanks,
Bill
> 
>> (define_expand "indirect_jump"
>> -  [(set (pc) (match_operand 0 "register_operand"))])
>> +  [(set (pc) (match_operand 0 "register_operand"))]
>> + ""
>> +{
>> +  /* We need to reserve a CR when forcing a mispredicted jump.  */
>> +  if (rs6000_safe_indirect_jumps) {
>> +    rtx ccreg = gen_reg_rtx (CCmode);
>> +    emit_insn (gen_rtx_SET (ccreg,
>> +			    gen_rtx_UNSPEC (CCmode,
>> +					    gen_rtvec (1, const0_rtx),
>> +					    UNSPEC_CRSET_EQ)));
>> +    rtvec v = rtvec_alloc (2);
>> +    RTVEC_ELT (v, 0) = operands[0];
>> +    RTVEC_ELT (v, 1) = ccreg;
>> +    emit_jump_insn (gen_rtx_SET (pc_rtx,
>> +				 gen_rtx_UNSPEC (Pmode, v,
>> +						 UNSPEC_COMP_GOTO_CR)));
>> +    DONE;
>> +  }
>> +})
>> 
>> (define_insn "*indirect_jump<mode>"
>>   [(set (pc)
>> 	(match_operand:P 0 "register_operand" "c,*l"))]
>> -  ""
>> +  "!rs6000_safe_indirect_jumps"
>>   "b%T0"
>>   [(set_attr "type" "jmpreg")])
>> 
>> +(define_insn "*indirect_jump<mode>_safe"
>> +  [(set (pc)
>> +	(unspec:P [(match_operand:P 0 "register_operand" "c,*l")
>> +		   (match_operand:CC 1 "cc_reg_operand" "y,y")]
>> +		   UNSPEC_COMP_GOTO_CR))]
>> +  "rs6000_safe_indirect_jumps"
>> +  "beq%T0- %1\;b ."
>> +  [(set_attr "type" "jmpreg")
>> +   (set_attr "length" "8")])
>> +
>> +(define_insn "*set_cr_eq"
>> +  [(set (match_operand:CC 0 "cc_reg_operand" "=y")
>> +	(unspec:CC [(const_int 0)] UNSPEC_CRSET_EQ))]
>> +  "rs6000_safe_indirect_jumps"
>> +  "crset %E0"
>> +  [(set_attr "type" "cr_logical")])
> 
> So merge this latter insn into the previous, making the CC a clobber?
> Like (not tested):
> 
> +(define_insn "indirect_jump<mode>_mispredict"
> +  [(set (pc)
> +	(match_operand:P 0 "register_operand" "c,*l")
> +   (clobber (match_operand:CC 1 "cc_reg_operand" "y,y"))]
> +  "rs6000_safe_indirect_jumps"
> +  "crset %E1\;beq%T0- %1\;b ."
> +  [(set_attr "type" "jmpreg")
> +   (set_attr "length" "12")])
> 
> and then change the indirect_jump pattern to simply select between the normal
> and mispredict patterns?
> 
>> +(define_expand "tablejumpsi_safe"
> 
> And then similar for tablejump.
> 
> 
> Segher
>

On Mon, Jan 15, 2018 at 11:54:41AM -0600, Bill Schmidt wrote:
> > I think we settled on calling the option -mmispredict-indirect-jumps;
> > please let me know if you still agree with that.  Or have thought of a
> > better name :-)
> 
> Looks like we are now looking at -m[no-]speculate-indirect-jumps with
> default to true.  LLVM folks are in agreement too.

A fine name!

> >> For the indirect jump, I don't see a way around it due to the
> >> expected form of indirect jumps in cfganal.c.
> > 
> > I'm not sure what you are getting at here, could you explain a bit?
> 
> An indirect jump is expected to be of the form (set (pc) (...other stuff));
> otherwise it might get missed and blocks that are only reachable from
> an indirect jump can get deleted.  I found this out when I tried to do
> something involving a parallel that was not very bright; that doesn't
> actually prevent anything if you are doing things right.  And the
> clobber solution you suggest should be just fine.

(rtlanal.c:computed_jump_p).  Yeah, PARALLELs work just fine.


Segher