Re: [PATCH, i386, MPX, 2/X] Pointers Checker [22/25] Target builtins

[Uros Bizjak <ubizjak at gmail dot com>]

Hello!

> 2013-11-15  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> * config/i386/i386-builtin-types.def (BND): New.
> (ULONG): New.
> (BND_FTYPE_PCVOID_ULONG): New.
> (VOID_FTYPE_BND_PCVOID): New.
> (VOID_FTYPE_PCVOID_PCVOID_BND): New.
> (BND_FTYPE_PCVOID_PCVOID): New.
> (BND_FTYPE_PCVOID): New.
> (BND_FTYPE_BND_BND): New.
> (PVOID_FTYPE_PVOID_PVOID_ULONG): New.
> (PVOID_FTYPE_PCVOID_BND_ULONG): New.
> (ULONG_FTYPE_VOID): New.
> (PVOID_FTYPE_BND): New.
> * config/i386/i386.c: Include tree-chkp.h, rtl-chkp.h.
> (ix86_builtins): Add
> IX86_BUILTIN_BNDMK, IX86_BUILTIN_BNDSTX,
> IX86_BUILTIN_BNDLDX, IX86_BUILTIN_BNDCL,
> IX86_BUILTIN_BNDCU, IX86_BUILTIN_BNDRET,
> IX86_BUILTIN_BNDSET, IX86_BUILTIN_BNDNARROW,
> IX86_BUILTIN_BNDINT, IX86_BUILTIN_ARG_BND,
> IX86_BUILTIN_SIZEOF, IX86_BUILTIN_BNDLOWER,
> IX86_BUILTIN_BNDUPPER.
> (builtin_isa): Add leaf_p and nothrow_p fields.
> (def_builtin): Initialize leaf_p and nothrow_p.
> (ix86_add_new_builtins): Handle leaf_p and nothrow_p
> flags.
> (bdesc_mpx): New.
> (bdesc_mpx_const): New.
> (ix86_init_mpx_builtins): New.
> (ix86_init_builtins): Call ix86_init_mpx_builtins.
> (ix86_expand_builtin): expand IX86_BUILTIN_BNDMK,
> IX86_BUILTIN_BNDSTX, IX86_BUILTIN_BNDLDX,
> IX86_BUILTIN_BNDCL, IX86_BUILTIN_BNDCU,
> IX86_BUILTIN_BNDRET, IX86_BUILTIN_BNDSET,
> IX86_BUILTIN_BNDNARROW, IX86_BUILTIN_BNDINT,
> IX86_BUILTIN_ARG_BND, IX86_BUILTIN_SIZEOF,
> IX86_BUILTIN_BNDLOWER, IX86_BUILTIN_BNDUPPER.

+      if (decl)
+ {
+  DECL_ATTRIBUTES (decl) = build_tree_list (get_identifier ("leaf"),
+    NULL_TREE);
+  TREE_NOTHROW (decl) = 1;
+ }
+      else
+ {
+  ix86_builtins_isa[(int)d->code].leaf_p = true;
+  ix86_builtins_isa[(int)d->code].nothrow_p = true;
+ }

Is there a reason these builtins need to be leaf and nothrow? It is
not clear from the source what is the reason. I'd suggest a comment
explaining this fact.

+      /* Builtin arg1 is size of block but instruction op1 should be
(size - 1).  */
+      op0 = expand_normal (arg0);
+      op1 = expand_normal (fold_build2 (PLUS_EXPR, TREE_TYPE (arg1),
+ arg1, integer_minus_one_node));
+      op0 = force_reg (Pmode, op0);
+      op1 = force_reg (Pmode, op1);

A tree expert should say if the above approach of operand fixups in
the patch is OK.

+      /* Avoid registers which connot be used as index.  */
+      if (REGNO (op1) == VIRTUAL_INCOMING_ARGS_REGNUM
+  || REGNO (op1) == VIRTUAL_STACK_VARS_REGNUM
+  || REGNO (op1) == VIRTUAL_OUTGOING_ARGS_REGNUM)

As said elsewhere, you want to check with index_register_operand predicate here.

+      /* If op1 was a register originally then it may have
+ mode other than Pmode.  We need to extend in such
+ case because bndldx may work only with Pmode regs.  */
+      if (GET_MODE (op1) != Pmode)
+ {
+  rtx ext = gen_rtx_ZERO_EXTEND (Pmode, op1);
+  op1 = gen_reg_rtx (Pmode);
+  emit_move_insn (op1, ext);
+ }

Please use ix86_zero_extend_to_Pmode.

+    case IX86_BUILTIN_BNDNARROW:
+      {
+ enum machine_mode mode = BNDmode;
+ enum machine_mode hmode = Pmode;

No need for static temporaries, use mode directly.

+ /* Generate mem expression to be used for access to LB and UB.  */
+ m1h1 = gen_rtx_MEM (hmode, XEXP (m1, 0));
+ m1h2 = gen_rtx_MEM (hmode,
+    gen_rtx_PLUS (Pmode, XEXP (m1, 0),
+  GEN_INT (GET_MODE_SIZE (hmode))));

Use subreg infrastructure here?

+ if (TARGET_CMOVE)
+  {
+    t2 = ix86_expand_compare (LTU, t1, lb);
+    emit_insn (gen_rtx_SET (VOIDmode, t1,
+    gen_rtx_IF_THEN_ELSE (hmode, t2, lb, t1)));
+  }
+ else
+  {
+    rtx nomove = gen_label_rtx ();
+    emit_cmp_and_jump_insns (t1, lb, GEU, const0_rtx, hmode, 1, nomove);
+    emit_insn (gen_rtx_SET (VOIDmode, t1, lb));
+    emit_label (nomove);
+  }
+ emit_move_insn (m1h1, t1);

This is used in a couple of places. Perhaps make a helper function?

+    m1 = assign_stack_local (mode, GET_MODE_SIZE (mode), 0);

Please use assign_i386_stack_local (mode, SLOT_TEMP)

+ m1h1 = gen_rtx_MEM (hmode, XEXP (m1, 0));
+ m1h2 = gen_rtx_MEM (hmode,
+    gen_rtx_PLUS (Pmode, XEXP (m1, 0),
+  GEN_INT (GET_MODE_SIZE (hmode))));
+ m2h1 = gen_rtx_MEM (hmode, XEXP (m2, 0));
+ m2h2 = gen_rtx_MEM (hmode,
+    gen_rtx_PLUS (Pmode, XEXP (m2, 0),
+  GEN_INT (GET_MODE_SIZE (hmode))));
+ rh1 = gen_rtx_MEM (hmode, XEXP (res, 0));
+ rh2 = gen_rtx_MEM (hmode,
+   gen_rtx_PLUS (Pmode, XEXP (res, 0),
+ GEN_INT (GET_MODE_SIZE (hmode))));

subergs, or plus_constant.

Uros.