Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On 10 Oct 21:20, Ilya Enkovich wrote: 2014-10-10 20:45 GMT+04:00 Jeff Law l...@redhat.com: On 10/09/14 10:54, Uros Bizjak wrote: On Thu, Oct 9, 2014 at 4:07 PM, Ilya Enkovich enkovich@gmail.com wrote: It appeared I changed a semantics of BNDMK expand when replaced tree operations with rtl ones. Original code: + op1 = expand_normal (fold_build2 (PLUS_EXPR, TREE_TYPE (arg1), + arg1, integer_minus_one_node)); + op1 = force_reg (Pmode, op1); Modified code: + op1 = expand_normal (arg1); + + if (!register_operand (op1, Pmode)) + op1 = ix86_zero_extend_to_Pmode (op1); + + /* Builtin arg1 is size of block but instruction op1 should +be (size - 1). */ + op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, +op1, 1, OPTAB_DIRECT); The problem is that in the fixed version we may modify value of a pseudo register into which arg1 is expanded which means incorrect value for all following usages of arg1. Didn't reveal it early because programs surprisingly rarely hit this bug. I do following change to fix it: op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, -op1, 1, OPTAB_DIRECT); +NULL, 1, OPTAB_DIRECT); Similar problem was also fixed for BNDNARROW. Does it look OK? I'm not aware of this type of limitation, and there are quite some similar constructs in i386.c. It is hard to say without the testcase what happens, perhaps one of RTX experts (CC'd) can advise what is recommended here. The problem is the call to expand_simple_binop. The source (op1) and the destination (op1) are obviously the same, so its going to clobber whatever value is in there. If there are other uses of the original value of op1, then things aren't going to work. But I'm a little unclear how there's be other later uses of that value. Perhaps Ilya could comment on that. op1 is a result of expand_normal called for SSA name. Other uses of op1 come from expand of uses of this SSA name in GIMPLE code. Regardless, unless there's a strong reason to do so, I'd generally recommend passing a NULL_RTX as the target for expansions so that you always get a new pseudo. Lots of optimizers in the RTL world work better if we don't have pseudos with multiple assignments. By passing NULL_RTX for the target we get that property more often. So a change like Ilya suggests (though I'd use NULL_RTX rather than NULL) makes sense. Will replace it with NULL_RTX. Thanks, Ilya Jeff Here is a version with NULL_RTX used instead of NULL. Thanks, Ilya -- 2014-10-14 Ilya Enkovich ilya.enkov...@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_BNDNARROW, IX86_BUILTIN_BNDINT, 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_emit_cmove): New. (ix86_emit_move_max): New. (ix86_expand_builtin): Expand IX86_BUILTIN_BNDMK, IX86_BUILTIN_BNDSTX, IX86_BUILTIN_BNDLDX, IX86_BUILTIN_BNDCL, IX86_BUILTIN_BNDCU, IX86_BUILTIN_BNDRET, IX86_BUILTIN_BNDNARROW, IX86_BUILTIN_BNDINT, IX86_BUILTIN_SIZEOF, IX86_BUILTIN_BNDLOWER, IX86_BUILTIN_BNDUPPER. diff --git a/gcc/config/i386/i386-builtin-types.def b/gcc/config/i386/i386-builtin-types.def index 9161287..5421ba9 100644 --- a/gcc/config/i386/i386-builtin-types.def +++ b/gcc/config/i386/i386-builtin-types.def @@ -47,6 +47,7 @@ DEF_PRIMITIVE_TYPE (UCHAR, unsigned_char_type_node) DEF_PRIMITIVE_TYPE (QI, char_type_node) DEF_PRIMITIVE_TYPE (HI, intHI_type_node) DEF_PRIMITIVE_TYPE (SI, intSI_type_node) +DEF_PRIMITIVE_TYPE (BND, pointer_bounds_type_node) # ??? Logically this should be intDI_type_node, but that maps to long # with 64-bit, and that's not how the emmintrin.h is written. Again,
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On 10/09/14 10:54, Uros Bizjak wrote: On Thu, Oct 9, 2014 at 4:07 PM, Ilya Enkovich enkovich@gmail.com wrote: It appeared I changed a semantics of BNDMK expand when replaced tree operations with rtl ones. Original code: + op1 = expand_normal (fold_build2 (PLUS_EXPR, TREE_TYPE (arg1), + arg1, integer_minus_one_node)); + op1 = force_reg (Pmode, op1); Modified code: + op1 = expand_normal (arg1); + + if (!register_operand (op1, Pmode)) + op1 = ix86_zero_extend_to_Pmode (op1); + + /* Builtin arg1 is size of block but instruction op1 should +be (size - 1). */ + op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, +op1, 1, OPTAB_DIRECT); The problem is that in the fixed version we may modify value of a pseudo register into which arg1 is expanded which means incorrect value for all following usages of arg1. Didn't reveal it early because programs surprisingly rarely hit this bug. I do following change to fix it: op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, -op1, 1, OPTAB_DIRECT); +NULL, 1, OPTAB_DIRECT); Similar problem was also fixed for BNDNARROW. Does it look OK? I'm not aware of this type of limitation, and there are quite some similar constructs in i386.c. It is hard to say without the testcase what happens, perhaps one of RTX experts (CC'd) can advise what is recommended here. The problem is the call to expand_simple_binop. The source (op1) and the destination (op1) are obviously the same, so its going to clobber whatever value is in there. If there are other uses of the original value of op1, then things aren't going to work. But I'm a little unclear how there's be other later uses of that value. Perhaps Ilya could comment on that. Regardless, unless there's a strong reason to do so, I'd generally recommend passing a NULL_RTX as the target for expansions so that you always get a new pseudo. Lots of optimizers in the RTL world work better if we don't have pseudos with multiple assignments. By passing NULL_RTX for the target we get that property more often. So a change like Ilya suggests (though I'd use NULL_RTX rather than NULL) makes sense. Jeff
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
2014-10-10 20:45 GMT+04:00 Jeff Law l...@redhat.com: On 10/09/14 10:54, Uros Bizjak wrote: On Thu, Oct 9, 2014 at 4:07 PM, Ilya Enkovich enkovich@gmail.com wrote: It appeared I changed a semantics of BNDMK expand when replaced tree operations with rtl ones. Original code: + op1 = expand_normal (fold_build2 (PLUS_EXPR, TREE_TYPE (arg1), + arg1, integer_minus_one_node)); + op1 = force_reg (Pmode, op1); Modified code: + op1 = expand_normal (arg1); + + if (!register_operand (op1, Pmode)) + op1 = ix86_zero_extend_to_Pmode (op1); + + /* Builtin arg1 is size of block but instruction op1 should +be (size - 1). */ + op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, +op1, 1, OPTAB_DIRECT); The problem is that in the fixed version we may modify value of a pseudo register into which arg1 is expanded which means incorrect value for all following usages of arg1. Didn't reveal it early because programs surprisingly rarely hit this bug. I do following change to fix it: op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, -op1, 1, OPTAB_DIRECT); +NULL, 1, OPTAB_DIRECT); Similar problem was also fixed for BNDNARROW. Does it look OK? I'm not aware of this type of limitation, and there are quite some similar constructs in i386.c. It is hard to say without the testcase what happens, perhaps one of RTX experts (CC'd) can advise what is recommended here. The problem is the call to expand_simple_binop. The source (op1) and the destination (op1) are obviously the same, so its going to clobber whatever value is in there. If there are other uses of the original value of op1, then things aren't going to work. But I'm a little unclear how there's be other later uses of that value. Perhaps Ilya could comment on that. op1 is a result of expand_normal called for SSA name. Other uses of op1 come from expand of uses of this SSA name in GIMPLE code. Regardless, unless there's a strong reason to do so, I'd generally recommend passing a NULL_RTX as the target for expansions so that you always get a new pseudo. Lots of optimizers in the RTL world work better if we don't have pseudos with multiple assignments. By passing NULL_RTX for the target we get that property more often. So a change like Ilya suggests (though I'd use NULL_RTX rather than NULL) makes sense. Will replace it with NULL_RTX. Thanks, Ilya Jeff
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
It appeared I changed a semantics of BNDMK expand when replaced tree operations
with rtl ones.
Original code:
+ op1 = expand_normal (fold_build2 (PLUS_EXPR, TREE_TYPE (arg1),
+ arg1, integer_minus_one_node));
+ op1 = force_reg (Pmode, op1);
Modified code:
+ op1 = expand_normal (arg1);
+
+ if (!register_operand (op1, Pmode))
+ op1 = ix86_zero_extend_to_Pmode (op1);
+
+ /* Builtin arg1 is size of block but instruction op1 should
+be (size - 1). */
+ op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx,
+op1, 1, OPTAB_DIRECT);
The problem is that in the fixed version we may modify value of a pseudo
register into which arg1 is expanded which means incorrect value for all
following usages of arg1.
Didn't reveal it early because programs surprisingly rarely hit this bug. I do
following change to fix it:
op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx,
-op1, 1, OPTAB_DIRECT);
+NULL, 1, OPTAB_DIRECT);
Similar problem was also fixed for BNDNARROW. Does it look OK?
Thanks,
Ilya
--
diff --git a/gcc/config/i386/i386-builtin-types.def
b/gcc/config/i386/i386-builtin-types.def
index 9161287..5421ba9 100644
--- a/gcc/config/i386/i386-builtin-types.def
+++ b/gcc/config/i386/i386-builtin-types.def
@@ -47,6 +47,7 @@ DEF_PRIMITIVE_TYPE (UCHAR, unsigned_char_type_node)
DEF_PRIMITIVE_TYPE (QI, char_type_node)
DEF_PRIMITIVE_TYPE (HI, intHI_type_node)
DEF_PRIMITIVE_TYPE (SI, intSI_type_node)
+DEF_PRIMITIVE_TYPE (BND, pointer_bounds_type_node)
# ??? Logically this should be intDI_type_node, but that maps to long
# with 64-bit, and that's not how the emmintrin.h is written. Again,
# changing this would change name mangling.
@@ -60,6 +61,7 @@ DEF_PRIMITIVE_TYPE (USHORT, short_unsigned_type_node)
DEF_PRIMITIVE_TYPE (INT, integer_type_node)
DEF_PRIMITIVE_TYPE (UINT, unsigned_type_node)
DEF_PRIMITIVE_TYPE (UNSIGNED, unsigned_type_node)
+DEF_PRIMITIVE_TYPE (ULONG, long_unsigned_type_node)
DEF_PRIMITIVE_TYPE (LONGLONG, long_long_integer_type_node)
DEF_PRIMITIVE_TYPE (ULONGLONG, long_long_unsigned_type_node)
DEF_PRIMITIVE_TYPE (UINT8, unsigned_char_type_node)
@@ -810,3 +812,15 @@ DEF_FUNCTION_TYPE_ALIAS (V2DI_FTYPE_V2DI_V2DI, TF)
DEF_FUNCTION_TYPE_ALIAS (V4SF_FTYPE_V4SF_V4SF, TF)
DEF_FUNCTION_TYPE_ALIAS (V4SI_FTYPE_V4SI_V4SI, TF)
DEF_FUNCTION_TYPE_ALIAS (V8HI_FTYPE_V8HI_V8HI, TF)
+
+# MPX builtins
+DEF_FUNCTION_TYPE (BND, PCVOID, ULONG)
+DEF_FUNCTION_TYPE (VOID, PCVOID, BND)
+DEF_FUNCTION_TYPE (VOID, PCVOID, BND, PCVOID)
+DEF_FUNCTION_TYPE (BND, PCVOID, PCVOID)
+DEF_FUNCTION_TYPE (BND, PCVOID)
+DEF_FUNCTION_TYPE (BND, BND, BND)
+DEF_FUNCTION_TYPE (PVOID, PVOID, PVOID, ULONG)
+DEF_FUNCTION_TYPE (PVOID, PCVOID, BND, ULONG)
+DEF_FUNCTION_TYPE (ULONG, VOID)
+DEF_FUNCTION_TYPE (PVOID, BND)
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 2b39ff1..3f464de 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -84,6 +84,8 @@ along with GCC; see the file COPYING3. If not see
#include tree-vectorizer.h
#include shrink-wrap.h
#include builtins.h
+#include tree-chkp.h
+#include rtl-chkp.h
static rtx legitimize_dllimport_symbol (rtx, bool);
static rtx legitimize_pe_coff_extern_decl (rtx, bool);
@@ -28776,6 +28778,19 @@ enum ix86_builtins
IX86_BUILTIN_XABORT,
IX86_BUILTIN_XTEST,
+ /* MPX */
+ IX86_BUILTIN_BNDMK,
+ IX86_BUILTIN_BNDSTX,
+ IX86_BUILTIN_BNDLDX,
+ IX86_BUILTIN_BNDCL,
+ IX86_BUILTIN_BNDCU,
+ IX86_BUILTIN_BNDRET,
+ IX86_BUILTIN_BNDNARROW,
+ IX86_BUILTIN_BNDINT,
+ IX86_BUILTIN_SIZEOF,
+ IX86_BUILTIN_BNDLOWER,
+ IX86_BUILTIN_BNDUPPER,
+
/* BMI instructions. */
IX86_BUILTIN_BEXTR32,
IX86_BUILTIN_BEXTR64,
@@ -28853,6 +28868,8 @@ struct builtin_isa {
enum ix86_builtin_func_type tcode; /* type to use in the declaration */
HOST_WIDE_INT isa; /* isa_flags this builtin is defined for */
bool const_p;/* true if the declaration is constant
*/
+ bool leaf_p; /* true if the declaration has leaf attribute */
+ bool nothrow_p; /* true if the declaration has nothrow
attribute */
bool set_and_not_built_p;
};
@@ -28904,6 +28921,8 @@ def_builtin (HOST_WIDE_INT mask, const char *name,
ix86_builtins[(int) code] = NULL_TREE;
ix86_builtins_isa[(int) code].tcode = tcode;
ix86_builtins_isa[(int) code].name = name;
+ ix86_builtins_isa[(int) code].leaf_p = false;
+ ix86_builtins_isa[(int) code].nothrow_p = false;
ix86_builtins_isa[(int) code].const_p = false;
ix86_builtins_isa[(int) code].set_and_not_built_p = true;
}
@@ -28954,6 +28973,11 @@ ix86_add_new_builtins (HOST_WIDE_INT isa)
ix86_builtins[i] = decl;
if (ix86_builtins_isa[i].const_p)
TREE_READONLY (decl) = 1;
+ if
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On Thu, Oct 9, 2014 at 4:07 PM, Ilya Enkovich enkovich@gmail.com wrote: It appeared I changed a semantics of BNDMK expand when replaced tree operations with rtl ones. Original code: + op1 = expand_normal (fold_build2 (PLUS_EXPR, TREE_TYPE (arg1), + arg1, integer_minus_one_node)); + op1 = force_reg (Pmode, op1); Modified code: + op1 = expand_normal (arg1); + + if (!register_operand (op1, Pmode)) + op1 = ix86_zero_extend_to_Pmode (op1); + + /* Builtin arg1 is size of block but instruction op1 should +be (size - 1). */ + op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, +op1, 1, OPTAB_DIRECT); The problem is that in the fixed version we may modify value of a pseudo register into which arg1 is expanded which means incorrect value for all following usages of arg1. Didn't reveal it early because programs surprisingly rarely hit this bug. I do following change to fix it: op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx, -op1, 1, OPTAB_DIRECT); +NULL, 1, OPTAB_DIRECT); Similar problem was also fixed for BNDNARROW. Does it look OK? I'm not aware of this type of limitation, and there are quite some similar constructs in i386.c. It is hard to say without the testcase what happens, perhaps one of RTX experts (CC'd) can advise what is recommended here. Uros.
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On 18 Sep 19:33, Uros Bizjak wrote:
On Thu, Sep 18, 2014 at 3:47 PM, Ilya Enkovich enkovich@gmail.com wrote:
Thanks for your comments. Below is a fixed verison.
Ilya
--
OK with a few nits below.
Thanks,
Uros.
+
+case IX86_BUILTIN_BNDRET:
+ arg0 = CALL_EXPR_ARG (exp, 0);
+ gcc_assert (TREE_CODE (arg0) == SSA_NAME);
+ target = chkp_get_rtl_bounds (arg0);
Please add vertical space here ...
+ /* If no bounds were specified for returned value,
+then use INIT bounds. It usually happens when
+some built-in function is expanded. */
+ if (!target)
+ {
+ rtx t1 = gen_reg_rtx (Pmode);
+ rtx t2 = gen_reg_rtx (Pmode);
+ target = gen_reg_rtx (BNDmode);
+ emit_move_insn (t1, const0_rtx);
+ emit_move_insn (t2, constm1_rtx);
+ emit_insn (BNDmode == BND64mode
+? gen_bnd64_mk (target, t1, t2)
+: gen_bnd32_mk (target, t1, t2));
+ }
... and here.
+ gcc_assert (target REG_P (target));
+ return target;
+
+
+ /* Generate mem expression to access UB. */
+ hmem = adjust_address (mem, Pmode, GET_MODE_SIZE (Pmode));
Vertical space here ...
+ /* We need to inverse all bits of UB. */
+ res = expand_simple_unop (Pmode, NOT, hmem, target, 1);
... and here.
+ if (res != target)
+ emit_move_insn (target, res);
+
Thank you for detailed review of this patch. Here is a final version.
Ilya
--
2014-09-19 Ilya Enkovich ilya.enkov...@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_BNDNARROW, IX86_BUILTIN_BNDINT,
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_emit_cmove): New.
(ix86_emit_move_max): New.
(ix86_expand_builtin): Expand IX86_BUILTIN_BNDMK,
IX86_BUILTIN_BNDSTX, IX86_BUILTIN_BNDLDX,
IX86_BUILTIN_BNDCL, IX86_BUILTIN_BNDCU,
IX86_BUILTIN_BNDRET, IX86_BUILTIN_BNDNARROW,
IX86_BUILTIN_BNDINT, IX86_BUILTIN_SIZEOF,
IX86_BUILTIN_BNDLOWER, IX86_BUILTIN_BNDUPPER.
diff --git a/gcc/config/i386/i386-builtin-types.def
b/gcc/config/i386/i386-builtin-types.def
index 35c0035..989297a 100644
--- a/gcc/config/i386/i386-builtin-types.def
+++ b/gcc/config/i386/i386-builtin-types.def
@@ -47,6 +47,7 @@ DEF_PRIMITIVE_TYPE (UCHAR, unsigned_char_type_node)
DEF_PRIMITIVE_TYPE (QI, char_type_node)
DEF_PRIMITIVE_TYPE (HI, intHI_type_node)
DEF_PRIMITIVE_TYPE (SI, intSI_type_node)
+DEF_PRIMITIVE_TYPE (BND, pointer_bounds_type_node)
# ??? Logically this should be intDI_type_node, but that maps to long
# with 64-bit, and that's not how the emmintrin.h is written. Again,
# changing this would change name mangling.
@@ -60,6 +61,7 @@ DEF_PRIMITIVE_TYPE (USHORT, short_unsigned_type_node)
DEF_PRIMITIVE_TYPE (INT, integer_type_node)
DEF_PRIMITIVE_TYPE (UINT, unsigned_type_node)
DEF_PRIMITIVE_TYPE (UNSIGNED, unsigned_type_node)
+DEF_PRIMITIVE_TYPE (ULONG, long_unsigned_type_node)
DEF_PRIMITIVE_TYPE (LONGLONG, long_long_integer_type_node)
DEF_PRIMITIVE_TYPE (ULONGLONG, long_long_unsigned_type_node)
DEF_PRIMITIVE_TYPE (UINT8, unsigned_char_type_node)
@@ -806,3 +808,15 @@ DEF_FUNCTION_TYPE_ALIAS (V2DI_FTYPE_V2DI_V2DI, TF)
DEF_FUNCTION_TYPE_ALIAS (V4SF_FTYPE_V4SF_V4SF, TF)
DEF_FUNCTION_TYPE_ALIAS (V4SI_FTYPE_V4SI_V4SI, TF)
DEF_FUNCTION_TYPE_ALIAS (V8HI_FTYPE_V8HI_V8HI, TF)
+
+# MPX builtins
+DEF_FUNCTION_TYPE (BND, PCVOID, ULONG)
+DEF_FUNCTION_TYPE (VOID, PCVOID, BND)
+DEF_FUNCTION_TYPE (VOID, PCVOID, BND, PCVOID)
+DEF_FUNCTION_TYPE (BND, PCVOID, PCVOID)
+DEF_FUNCTION_TYPE (BND, PCVOID)
+DEF_FUNCTION_TYPE (BND, BND, BND)
+DEF_FUNCTION_TYPE (PVOID, PVOID, PVOID, ULONG)
+DEF_FUNCTION_TYPE (PVOID, PCVOID, BND, ULONG)
+DEF_FUNCTION_TYPE (ULONG, VOID)
+DEF_FUNCTION_TYPE (PVOID, BND)
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On 17 Sep 20:06, Uros Bizjak wrote:
On Wed, Sep 17, 2014 at 6:31 PM, Ilya Enkovich enkovich@gmail.com wrote:
I don't like the way arguments are prepared. For the case above,
bnd_ldx should have index_register_operand predicate in its pattern,
and this predicate (and its mode) should be checked in the expander
code. There are many examples of argument expansion in
ix86_expand_builtin function, including how Pmode is handled.
Also, please see how target is handled there. Target can be null, so
REG_P predicate will crash.
You should also select insn patterns depending on BNDmode, not
TARGET_64BIT.
Please use assign_386_stack_local so stack slots can be shared.
SLOT_TEMP is intended for short-lived temporaries, you can introduce
new slots if you need more live values at once.
Uros.
Thanks for comments! Here is a new version in which I addressed all
your concerns.
Unfortunately, it doesn't. The patch only fixed one instance w.r.t to
target handling, the one I referred as an example. You still have
unchecked target, at least in IX86_BUILTIN_BNDMK.
However, you have a general problems in your builtin expansion code,
so please look at how other builtins are handled. E.g.:
if (optimize || !target
|| GET_MODE (target) != tmode
|| !register_operand(target, tmode))
target = gen_reg_rtx (tmode);
also, here is an example how input operands are prepared:
op0 = expand_normal (arg0);
op1 = expand_normal (arg1);
op2 = expand_normal (arg2);
if (!register_operand (op0, Pmode))
op0 = ix86_zero_extend_to_Pmode (op0);
if (!register_operand (op1, SImode))
op1 = copy_to_mode_reg (SImode, op1);
if (!register_operand (op2, SImode))
op2 = copy_to_mode_reg (SImode, op2);
So, Pmode is handled in a special way, even when x32 is not considered.
BTW: I wonder if word_mode is needed here, Pmode can be SImode with
address prefix (x32).
Inside the expanders, please use expand_simple_binop and expand_unop
on RTX, not tree expressions. Again, please see many examples.
Thank you for additional explanations. Hope this time I answer your
concerns correctly :)
Yes, this version is MUCH better. There are further comments down the code.
2014-09-17 Ilya Enkovich ilya.enkov...@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_BNDNARROW, IX86_BUILTIN_BNDINT,
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_emit_cmove): New.
(ix86_emit_move_max): New.
(ix86_expand_builtin): Expand IX86_BUILTIN_BNDMK,
IX86_BUILTIN_BNDSTX, IX86_BUILTIN_BNDLDX,
IX86_BUILTIN_BNDCL, IX86_BUILTIN_BNDCU,
IX86_BUILTIN_BNDRET, IX86_BUILTIN_BNDNARROW,
IX86_BUILTIN_BNDINT, IX86_BUILTIN_SIZEOF,
IX86_BUILTIN_BNDLOWER, IX86_BUILTIN_BNDUPPER.
* config/i386/i386.h (ix86_stack_slot): Added SLOT_BND_STORED.
..
+ /* We need to move bounds to memory before any computations. */
+ if (!MEM_P (op1))
+ {
+ m1 = assign_386_stack_local (BNDmode, SLOT_TEMP);
+ emit_move_insn (m1, op1);
+ }
+ else
+ m1 = op1;
No negative conditions, please. Just swap the arms of if sentence. It
is much more readable.
+
+ /* Generate mem expression to be used for access to LB and UB. */
+ m1h1 = gen_rtx_MEM (Pmode, XEXP (m1, 0));
+ m1h2 = gen_rtx_MEM (Pmode, plus_constant (Pmode, XEXP (m1, 0),
+ GET_MODE_SIZE (Pmode)));
Please use adjust_address instead of manually producing MEMs.
+
+ t1 = gen_reg_rtx (Pmode);
+
+ /* Compute LB. */
+ emit_move_insn (t1, m1h1);
+ ix86_emit_move_max (t1, lb);
+
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On Thu, Sep 18, 2014 at 3:47 PM, Ilya Enkovich enkovich@gmail.com wrote:
Thanks for your comments. Below is a fixed verison.
Ilya
--
2014-09-17 Ilya Enkovich ilya.enkov...@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_BNDNARROW, IX86_BUILTIN_BNDINT,
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_emit_cmove): New.
(ix86_emit_move_max): New.
(ix86_expand_builtin): Expand IX86_BUILTIN_BNDMK,
IX86_BUILTIN_BNDSTX, IX86_BUILTIN_BNDLDX,
IX86_BUILTIN_BNDCL, IX86_BUILTIN_BNDCU,
IX86_BUILTIN_BNDRET, IX86_BUILTIN_BNDNARROW,
IX86_BUILTIN_BNDINT, IX86_BUILTIN_SIZEOF,
IX86_BUILTIN_BNDLOWER, IX86_BUILTIN_BNDUPPER.
OK with a few nits below.
Thanks,
Uros.
diff --git a/gcc/config/i386/i386-builtin-types.def
b/gcc/config/i386/i386-builtin-types.def
index 35c0035..989297a 100644
--- a/gcc/config/i386/i386-builtin-types.def
+++ b/gcc/config/i386/i386-builtin-types.def
@@ -47,6 +47,7 @@ DEF_PRIMITIVE_TYPE (UCHAR, unsigned_char_type_node)
DEF_PRIMITIVE_TYPE (QI, char_type_node)
DEF_PRIMITIVE_TYPE (HI, intHI_type_node)
DEF_PRIMITIVE_TYPE (SI, intSI_type_node)
+DEF_PRIMITIVE_TYPE (BND, pointer_bounds_type_node)
# ??? Logically this should be intDI_type_node, but that maps to long
# with 64-bit, and that's not how the emmintrin.h is written. Again,
# changing this would change name mangling.
@@ -60,6 +61,7 @@ DEF_PRIMITIVE_TYPE (USHORT, short_unsigned_type_node)
DEF_PRIMITIVE_TYPE (INT, integer_type_node)
DEF_PRIMITIVE_TYPE (UINT, unsigned_type_node)
DEF_PRIMITIVE_TYPE (UNSIGNED, unsigned_type_node)
+DEF_PRIMITIVE_TYPE (ULONG, long_unsigned_type_node)
DEF_PRIMITIVE_TYPE (LONGLONG, long_long_integer_type_node)
DEF_PRIMITIVE_TYPE (ULONGLONG, long_long_unsigned_type_node)
DEF_PRIMITIVE_TYPE (UINT8, unsigned_char_type_node)
@@ -806,3 +808,15 @@ DEF_FUNCTION_TYPE_ALIAS (V2DI_FTYPE_V2DI_V2DI, TF)
DEF_FUNCTION_TYPE_ALIAS (V4SF_FTYPE_V4SF_V4SF, TF)
DEF_FUNCTION_TYPE_ALIAS (V4SI_FTYPE_V4SI_V4SI, TF)
DEF_FUNCTION_TYPE_ALIAS (V8HI_FTYPE_V8HI_V8HI, TF)
+
+# MPX builtins
+DEF_FUNCTION_TYPE (BND, PCVOID, ULONG)
+DEF_FUNCTION_TYPE (VOID, PCVOID, BND)
+DEF_FUNCTION_TYPE (VOID, PCVOID, BND, PCVOID)
+DEF_FUNCTION_TYPE (BND, PCVOID, PCVOID)
+DEF_FUNCTION_TYPE (BND, PCVOID)
+DEF_FUNCTION_TYPE (BND, BND, BND)
+DEF_FUNCTION_TYPE (PVOID, PVOID, PVOID, ULONG)
+DEF_FUNCTION_TYPE (PVOID, PCVOID, BND, ULONG)
+DEF_FUNCTION_TYPE (ULONG, VOID)
+DEF_FUNCTION_TYPE (PVOID, BND)
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index d0f58b1..6082f86 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -85,6 +85,8 @@ along with GCC; see the file COPYING3. If not see
#include tree-vectorizer.h
#include shrink-wrap.h
#include builtins.h
+#include tree-chkp.h
+#include rtl-chkp.h
static rtx legitimize_dllimport_symbol (rtx, bool);
static rtx legitimize_pe_coff_extern_decl (rtx, bool);
@@ -28775,6 +28777,19 @@ enum ix86_builtins
IX86_BUILTIN_XABORT,
IX86_BUILTIN_XTEST,
+ /* MPX */
+ IX86_BUILTIN_BNDMK,
+ IX86_BUILTIN_BNDSTX,
+ IX86_BUILTIN_BNDLDX,
+ IX86_BUILTIN_BNDCL,
+ IX86_BUILTIN_BNDCU,
+ IX86_BUILTIN_BNDRET,
+ IX86_BUILTIN_BNDNARROW,
+ IX86_BUILTIN_BNDINT,
+ IX86_BUILTIN_SIZEOF,
+ IX86_BUILTIN_BNDLOWER,
+ IX86_BUILTIN_BNDUPPER,
+
/* BMI instructions. */
IX86_BUILTIN_BEXTR32,
IX86_BUILTIN_BEXTR64,
@@ -28848,6 +28863,8 @@ struct builtin_isa {
enum ix86_builtin_func_type tcode; /* type to use in the declaration */
HOST_WIDE_INT isa; /* isa_flags this builtin is defined for */
bool const_p;/* true if the declaration is
constant */
+ bool leaf_p; /* true if the declaration has leaf attribute
*/
+ bool nothrow_p; /* true if the declaration has nothrow
attribute */
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On 16 Sep 12:02, Uros Bizjak wrote:
Hm, can this patch be compiled as part of the series? The expanders
refer to various gen_bnd patterns that I don't see. Also, I don't see
BND mode introduced.
Hi,
Here is a patch from the series that introduces modes and instructions:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00880.html. It needs update in
bndldx expander as you suggested.
Anyway, some general observations:
+case IX86_BUILTIN_BNDLDX:
+ arg0 = CALL_EXPR_ARG (exp, 0);
+ arg1 = CALL_EXPR_ARG (exp, 1);
+
+ op0 = expand_normal (arg0);
+ op1 = expand_normal (arg1);
+
+ op0 = force_reg (Pmode, op0);
+ op1 = force_reg (Pmode, op1);
+
+ /* Avoid registers which connot be used as index. */
+ if (!index_register_operand (op1, Pmode))
+ {
+ rtx temp = gen_reg_rtx (Pmode);
+ emit_move_insn (temp, op1);
+ op1 = temp;
+ }
+
+ /* 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)
+ op1 = ix86_zero_extend_to_Pmode (op1);
+
+ if (REG_P (target))
+ emit_insn (TARGET_64BIT
+ ? gen_bnd64_ldx (target, op0, op1)
+ : gen_bnd32_ldx (target, op0, op1));
+ else
+ {
+ rtx temp = gen_reg_rtx (BNDmode);
+ emit_insn (TARGET_64BIT
+? gen_bnd64_ldx (temp, op0, op1)
+: gen_bnd32_ldx (temp, op0, op1));
+ emit_move_insn (target, temp);
+ }
+ return target;
I don't like the way arguments are prepared. For the case above,
bnd_ldx should have index_register_operand predicate in its pattern,
and this predicate (and its mode) should be checked in the expander
code. There are many examples of argument expansion in
ix86_expand_builtin function, including how Pmode is handled.
Also, please see how target is handled there. Target can be null, so
REG_P predicate will crash.
You should also select insn patterns depending on BNDmode, not TARGET_64BIT.
Please use assign_386_stack_local so stack slots can be shared.
SLOT_TEMP is intended for short-lived temporaries, you can introduce
new slots if you need more live values at once.
Uros.
Thanks for comments! Here is a new version in which I addressed all your
concerns.
Ilya
--
2014-09-17 Ilya Enkovich ilya.enkov...@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_BNDNARROW, IX86_BUILTIN_BNDINT,
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_emit_move_max): New.
(ix86_expand_builtin): Expand IX86_BUILTIN_BNDMK,
IX86_BUILTIN_BNDSTX, IX86_BUILTIN_BNDLDX,
IX86_BUILTIN_BNDCL, IX86_BUILTIN_BNDCU,
IX86_BUILTIN_BNDRET, IX86_BUILTIN_BNDNARROW,
IX86_BUILTIN_BNDINT, IX86_BUILTIN_SIZEOF,
IX86_BUILTIN_BNDLOWER, IX86_BUILTIN_BNDUPPER.
* config/i386/i386.h (ix86_stack_slot): Added SLOT_BND_STORED.
diff --git a/gcc/config/i386/i386-builtin-types.def
b/gcc/config/i386/i386-builtin-types.def
index 35c0035..989297a 100644
--- a/gcc/config/i386/i386-builtin-types.def
+++ b/gcc/config/i386/i386-builtin-types.def
@@ -47,6 +47,7 @@ DEF_PRIMITIVE_TYPE (UCHAR, unsigned_char_type_node)
DEF_PRIMITIVE_TYPE (QI, char_type_node)
DEF_PRIMITIVE_TYPE (HI, intHI_type_node)
DEF_PRIMITIVE_TYPE (SI, intSI_type_node)
+DEF_PRIMITIVE_TYPE (BND, pointer_bounds_type_node)
# ??? Logically this should be intDI_type_node, but that maps to long
# with 64-bit, and that's not how the emmintrin.h is written. Again,
# changing this would change name mangling.
@@ -60,6 +61,7 @@ DEF_PRIMITIVE_TYPE (USHORT, short_unsigned_type_node)
DEF_PRIMITIVE_TYPE (INT, integer_type_node)
DEF_PRIMITIVE_TYPE (UINT, unsigned_type_node)
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On Wed, Sep 17, 2014 at 10:11 AM, Ilya Enkovich enkovich@gmail.com wrote:
On 16 Sep 12:02, Uros Bizjak wrote:
Hm, can this patch be compiled as part of the series? The expanders
refer to various gen_bnd patterns that I don't see. Also, I don't see
BND mode introduced.
Hi,
Here is a patch from the series that introduces modes and instructions:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00880.html. It needs update in
bndldx expander as you suggested.
Anyway, some general observations:
+case IX86_BUILTIN_BNDLDX:
+ arg0 = CALL_EXPR_ARG (exp, 0);
+ arg1 = CALL_EXPR_ARG (exp, 1);
+
+ op0 = expand_normal (arg0);
+ op1 = expand_normal (arg1);
+
+ op0 = force_reg (Pmode, op0);
+ op1 = force_reg (Pmode, op1);
+
+ /* Avoid registers which connot be used as index. */
+ if (!index_register_operand (op1, Pmode))
+ {
+ rtx temp = gen_reg_rtx (Pmode);
+ emit_move_insn (temp, op1);
+ op1 = temp;
+ }
+
+ /* 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)
+ op1 = ix86_zero_extend_to_Pmode (op1);
+
+ if (REG_P (target))
+ emit_insn (TARGET_64BIT
+ ? gen_bnd64_ldx (target, op0, op1)
+ : gen_bnd32_ldx (target, op0, op1));
+ else
+ {
+ rtx temp = gen_reg_rtx (BNDmode);
+ emit_insn (TARGET_64BIT
+? gen_bnd64_ldx (temp, op0, op1)
+: gen_bnd32_ldx (temp, op0, op1));
+ emit_move_insn (target, temp);
+ }
+ return target;
I don't like the way arguments are prepared. For the case above,
bnd_ldx should have index_register_operand predicate in its pattern,
and this predicate (and its mode) should be checked in the expander
code. There are many examples of argument expansion in
ix86_expand_builtin function, including how Pmode is handled.
Also, please see how target is handled there. Target can be null, so
REG_P predicate will crash.
You should also select insn patterns depending on BNDmode, not TARGET_64BIT.
Please use assign_386_stack_local so stack slots can be shared.
SLOT_TEMP is intended for short-lived temporaries, you can introduce
new slots if you need more live values at once.
Uros.
Thanks for comments! Here is a new version in which I addressed all your
concerns.
Unfortunately, it doesn't. The patch only fixed one instance w.r.t to
target handling, the one I referred as an example. You still have
unchecked target, at least in IX86_BUILTIN_BNDMK.
However, you have a general problems in your builtin expansion code,
so please look at how other builtins are handled. E.g.:
if (optimize || !target
|| GET_MODE (target) != tmode
|| !register_operand(target, tmode))
target = gen_reg_rtx (tmode);
also, here is an example how input operands are prepared:
op0 = expand_normal (arg0);
op1 = expand_normal (arg1);
op2 = expand_normal (arg2);
if (!register_operand (op0, Pmode))
op0 = ix86_zero_extend_to_Pmode (op0);
if (!register_operand (op1, SImode))
op1 = copy_to_mode_reg (SImode, op1);
if (!register_operand (op2, SImode))
op2 = copy_to_mode_reg (SImode, op2);
So, Pmode is handled in a special way, even when x32 is not considered.
BTW: I wonder if word_mode is needed here, Pmode can be SImode with
address prefix (x32).
Inside the expanders, please use expand_simple_binop and expand_unop
on RTX, not tree expressions. Again, please see many examples.
Please use emit_move_insn instead of
emit_insn (gen_move_insn (m1, op1));
As a wish, can you perhaps write a generic cmove expander to be also
used in other places?
+static void
+ix86_emit_move_max (rtx dst, rtx src)
Uros.
Ilya
--
2014-09-17 Ilya Enkovich ilya.enkov...@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_BNDNARROW, IX86_BUILTIN_BNDINT,
IX86_BUILTIN_SIZEOF, IX86_BUILTIN_BNDLOWER,
IX86_BUILTIN_BNDUPPER.
(builtin_isa): Add leaf_p and nothrow_p fields.
(def_builtin):
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On 17 Sep 11:42, Uros Bizjak wrote:
On Wed, Sep 17, 2014 at 10:11 AM, Ilya Enkovich enkovich@gmail.com
wrote:
On 16 Sep 12:02, Uros Bizjak wrote:
Hm, can this patch be compiled as part of the series? The expanders
refer to various gen_bnd patterns that I don't see. Also, I don't see
BND mode introduced.
Hi,
Here is a patch from the series that introduces modes and instructions:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00880.html. It needs update
in bndldx expander as you suggested.
Anyway, some general observations:
+case IX86_BUILTIN_BNDLDX:
+ arg0 = CALL_EXPR_ARG (exp, 0);
+ arg1 = CALL_EXPR_ARG (exp, 1);
+
+ op0 = expand_normal (arg0);
+ op1 = expand_normal (arg1);
+
+ op0 = force_reg (Pmode, op0);
+ op1 = force_reg (Pmode, op1);
+
+ /* Avoid registers which connot be used as index. */
+ if (!index_register_operand (op1, Pmode))
+ {
+ rtx temp = gen_reg_rtx (Pmode);
+ emit_move_insn (temp, op1);
+ op1 = temp;
+ }
+
+ /* 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)
+ op1 = ix86_zero_extend_to_Pmode (op1);
+
+ if (REG_P (target))
+ emit_insn (TARGET_64BIT
+ ? gen_bnd64_ldx (target, op0, op1)
+ : gen_bnd32_ldx (target, op0, op1));
+ else
+ {
+ rtx temp = gen_reg_rtx (BNDmode);
+ emit_insn (TARGET_64BIT
+? gen_bnd64_ldx (temp, op0, op1)
+: gen_bnd32_ldx (temp, op0, op1));
+ emit_move_insn (target, temp);
+ }
+ return target;
I don't like the way arguments are prepared. For the case above,
bnd_ldx should have index_register_operand predicate in its pattern,
and this predicate (and its mode) should be checked in the expander
code. There are many examples of argument expansion in
ix86_expand_builtin function, including how Pmode is handled.
Also, please see how target is handled there. Target can be null, so
REG_P predicate will crash.
You should also select insn patterns depending on BNDmode, not
TARGET_64BIT.
Please use assign_386_stack_local so stack slots can be shared.
SLOT_TEMP is intended for short-lived temporaries, you can introduce
new slots if you need more live values at once.
Uros.
Thanks for comments! Here is a new version in which I addressed all your
concerns.
Unfortunately, it doesn't. The patch only fixed one instance w.r.t to
target handling, the one I referred as an example. You still have
unchecked target, at least in IX86_BUILTIN_BNDMK.
However, you have a general problems in your builtin expansion code,
so please look at how other builtins are handled. E.g.:
if (optimize || !target
|| GET_MODE (target) != tmode
|| !register_operand(target, tmode))
target = gen_reg_rtx (tmode);
also, here is an example how input operands are prepared:
op0 = expand_normal (arg0);
op1 = expand_normal (arg1);
op2 = expand_normal (arg2);
if (!register_operand (op0, Pmode))
op0 = ix86_zero_extend_to_Pmode (op0);
if (!register_operand (op1, SImode))
op1 = copy_to_mode_reg (SImode, op1);
if (!register_operand (op2, SImode))
op2 = copy_to_mode_reg (SImode, op2);
So, Pmode is handled in a special way, even when x32 is not considered.
BTW: I wonder if word_mode is needed here, Pmode can be SImode with
address prefix (x32).
Inside the expanders, please use expand_simple_binop and expand_unop
on RTX, not tree expressions. Again, please see many examples.
Thank you for additional explanations. Hope this time I answer your concerns
correctly :)
Please use emit_move_insn instead of
emit_insn (gen_move_insn (m1, op1));
As a wish, can you perhaps write a generic cmove expander to be also
used in other places?
I added ix86_emit_cmove as a general form of ix86_emit_move_max.
+static void
+ix86_emit_move_max (rtx dst, rtx src)
Uros.
Thanks,
Ilya
--
2014-09-17 Ilya Enkovich ilya.enkov...@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,
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
On Wed, Sep 17, 2014 at 6:31 PM, Ilya Enkovich enkovich@gmail.com wrote:
I don't like the way arguments are prepared. For the case above,
bnd_ldx should have index_register_operand predicate in its pattern,
and this predicate (and its mode) should be checked in the expander
code. There are many examples of argument expansion in
ix86_expand_builtin function, including how Pmode is handled.
Also, please see how target is handled there. Target can be null, so
REG_P predicate will crash.
You should also select insn patterns depending on BNDmode, not
TARGET_64BIT.
Please use assign_386_stack_local so stack slots can be shared.
SLOT_TEMP is intended for short-lived temporaries, you can introduce
new slots if you need more live values at once.
Uros.
Thanks for comments! Here is a new version in which I addressed all your
concerns.
Unfortunately, it doesn't. The patch only fixed one instance w.r.t to
target handling, the one I referred as an example. You still have
unchecked target, at least in IX86_BUILTIN_BNDMK.
However, you have a general problems in your builtin expansion code,
so please look at how other builtins are handled. E.g.:
if (optimize || !target
|| GET_MODE (target) != tmode
|| !register_operand(target, tmode))
target = gen_reg_rtx (tmode);
also, here is an example how input operands are prepared:
op0 = expand_normal (arg0);
op1 = expand_normal (arg1);
op2 = expand_normal (arg2);
if (!register_operand (op0, Pmode))
op0 = ix86_zero_extend_to_Pmode (op0);
if (!register_operand (op1, SImode))
op1 = copy_to_mode_reg (SImode, op1);
if (!register_operand (op2, SImode))
op2 = copy_to_mode_reg (SImode, op2);
So, Pmode is handled in a special way, even when x32 is not considered.
BTW: I wonder if word_mode is needed here, Pmode can be SImode with
address prefix (x32).
Inside the expanders, please use expand_simple_binop and expand_unop
on RTX, not tree expressions. Again, please see many examples.
Thank you for additional explanations. Hope this time I answer your concerns
correctly :)
Yes, this version is MUCH better. There are further comments down the code.
2014-09-17 Ilya Enkovich ilya.enkov...@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_BNDNARROW, IX86_BUILTIN_BNDINT,
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_emit_cmove): New.
(ix86_emit_move_max): New.
(ix86_expand_builtin): Expand IX86_BUILTIN_BNDMK,
IX86_BUILTIN_BNDSTX, IX86_BUILTIN_BNDLDX,
IX86_BUILTIN_BNDCL, IX86_BUILTIN_BNDCU,
IX86_BUILTIN_BNDRET, IX86_BUILTIN_BNDNARROW,
IX86_BUILTIN_BNDINT, IX86_BUILTIN_SIZEOF,
IX86_BUILTIN_BNDLOWER, IX86_BUILTIN_BNDUPPER.
* config/i386/i386.h (ix86_stack_slot): Added SLOT_BND_STORED.
..
+ /* We need to move bounds to memory before any computations. */
+ if (!MEM_P (op1))
+ {
+ m1 = assign_386_stack_local (BNDmode, SLOT_TEMP);
+ emit_move_insn (m1, op1);
+ }
+ else
+ m1 = op1;
No negative conditions, please. Just swap the arms of if sentence. It
is much more readable.
+
+ /* Generate mem expression to be used for access to LB and UB. */
+ m1h1 = gen_rtx_MEM (Pmode, XEXP (m1, 0));
+ m1h2 = gen_rtx_MEM (Pmode, plus_constant (Pmode, XEXP (m1, 0),
+ GET_MODE_SIZE (Pmode)));
Please use adjust_address instead of manually producing MEMs.
+
+ t1 = gen_reg_rtx (Pmode);
+
+ /* Compute LB. */
+ emit_move_insn (t1, m1h1);
+ ix86_emit_move_max (t1, lb);
+ emit_move_insn (m1h1, t1);
+
+ /* Compute UB. UB is stored in 1's complement form. Therefore
+ we also use max here. */
+ emit_move_insn
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
2014-06-11 Ilya Enkovich ilya.enkov...@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_BNDNARROW, IX86_BUILTIN_BNDINT,
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_emit_move_max): New.
(ix86_expand_builtin): Expand IX86_BUILTIN_BNDMK,
IX86_BUILTIN_BNDSTX, IX86_BUILTIN_BNDLDX,
IX86_BUILTIN_BNDCL, IX86_BUILTIN_BNDCU,
IX86_BUILTIN_BNDRET, IX86_BUILTIN_BNDNARROW,
IX86_BUILTIN_BNDINT, IX86_BUILTIN_SIZEOF,
IX86_BUILTIN_BNDLOWER, IX86_BUILTIN_BNDUPPER.
Hm, can this patch be compiled as part of the series? The expanders
refer to various gen_bnd patterns that I don't see. Also, I don't see
BND mode introduced.
Anyway, some general observations:
+case IX86_BUILTIN_BNDLDX:
+ arg0 = CALL_EXPR_ARG (exp, 0);
+ arg1 = CALL_EXPR_ARG (exp, 1);
+
+ op0 = expand_normal (arg0);
+ op1 = expand_normal (arg1);
+
+ op0 = force_reg (Pmode, op0);
+ op1 = force_reg (Pmode, op1);
+
+ /* Avoid registers which connot be used as index. */
+ if (!index_register_operand (op1, Pmode))
+ {
+ rtx temp = gen_reg_rtx (Pmode);
+ emit_move_insn (temp, op1);
+ op1 = temp;
+ }
+
+ /* 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)
+ op1 = ix86_zero_extend_to_Pmode (op1);
+
+ if (REG_P (target))
+ emit_insn (TARGET_64BIT
+ ? gen_bnd64_ldx (target, op0, op1)
+ : gen_bnd32_ldx (target, op0, op1));
+ else
+ {
+ rtx temp = gen_reg_rtx (BNDmode);
+ emit_insn (TARGET_64BIT
+? gen_bnd64_ldx (temp, op0, op1)
+: gen_bnd32_ldx (temp, op0, op1));
+ emit_move_insn (target, temp);
+ }
+ return target;
I don't like the way arguments are prepared. For the case above,
bnd_ldx should have index_register_operand predicate in its pattern,
and this predicate (and its mode) should be checked in the expander
code. There are many examples of argument expansion in
ix86_expand_builtin function, including how Pmode is handled.
Also, please see how target is handled there. Target can be null, so
REG_P predicate will crash.
You should also select insn patterns depending on BNDmode, not TARGET_64BIT.
Please use assign_386_stack_local so stack slots can be shared.
SLOT_TEMP is intended for short-lived temporaries, you can introduce
new slots if you need more live values at once.
Uros.
Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target
Ping
2014-06-11 17:58 GMT+04:00 Ilya Enkovich enkovich@gmail.com:
Hi,
This patch adds i386 target builtins for Pointer Bounds Checker.
Bootstrapped and tested on linux-x86_64.
Thanks,
Ilya
--
gcc/
2014-06-11 Ilya Enkovich ilya.enkov...@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_BNDNARROW, IX86_BUILTIN_BNDINT,
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_emit_move_max): New.
(ix86_expand_builtin): Expand IX86_BUILTIN_BNDMK,
IX86_BUILTIN_BNDSTX, IX86_BUILTIN_BNDLDX,
IX86_BUILTIN_BNDCL, IX86_BUILTIN_BNDCU,
IX86_BUILTIN_BNDRET, IX86_BUILTIN_BNDNARROW,
IX86_BUILTIN_BNDINT, IX86_BUILTIN_SIZEOF,
IX86_BUILTIN_BNDLOWER, IX86_BUILTIN_BNDUPPER.
diff --git a/gcc/config/i386/i386-builtin-types.def
b/gcc/config/i386/i386-builtin-types.def
index 822c5e5..169d40a 100644
--- a/gcc/config/i386/i386-builtin-types.def
+++ b/gcc/config/i386/i386-builtin-types.def
@@ -47,6 +47,7 @@ DEF_PRIMITIVE_TYPE (UCHAR, unsigned_char_type_node)
DEF_PRIMITIVE_TYPE (QI, char_type_node)
DEF_PRIMITIVE_TYPE (HI, intHI_type_node)
DEF_PRIMITIVE_TYPE (SI, intSI_type_node)
+DEF_PRIMITIVE_TYPE (BND, pointer_bounds_type_node)
# ??? Logically this should be intDI_type_node, but that maps to long
# with 64-bit, and that's not how the emmintrin.h is written. Again,
# changing this would change name mangling.
@@ -60,6 +61,7 @@ DEF_PRIMITIVE_TYPE (USHORT, short_unsigned_type_node)
DEF_PRIMITIVE_TYPE (INT, integer_type_node)
DEF_PRIMITIVE_TYPE (UINT, unsigned_type_node)
DEF_PRIMITIVE_TYPE (UNSIGNED, unsigned_type_node)
+DEF_PRIMITIVE_TYPE (ULONG, long_unsigned_type_node)
DEF_PRIMITIVE_TYPE (LONGLONG, long_long_integer_type_node)
DEF_PRIMITIVE_TYPE (ULONGLONG, long_long_unsigned_type_node)
DEF_PRIMITIVE_TYPE (UINT8, unsigned_char_type_node)
@@ -806,3 +808,15 @@ DEF_FUNCTION_TYPE_ALIAS (V2DI_FTYPE_V2DI_V2DI, TF)
DEF_FUNCTION_TYPE_ALIAS (V4SF_FTYPE_V4SF_V4SF, TF)
DEF_FUNCTION_TYPE_ALIAS (V4SI_FTYPE_V4SI_V4SI, TF)
DEF_FUNCTION_TYPE_ALIAS (V8HI_FTYPE_V8HI_V8HI, TF)
+
+# MPX builtins
+DEF_FUNCTION_TYPE (BND, PCVOID, ULONG)
+DEF_FUNCTION_TYPE (VOID, PCVOID, BND)
+DEF_FUNCTION_TYPE (VOID, PCVOID, BND, PCVOID)
+DEF_FUNCTION_TYPE (BND, PCVOID, PCVOID)
+DEF_FUNCTION_TYPE (BND, PCVOID)
+DEF_FUNCTION_TYPE (BND, BND, BND)
+DEF_FUNCTION_TYPE (PVOID, PVOID, PVOID, ULONG)
+DEF_FUNCTION_TYPE (PVOID, PCVOID, BND, ULONG)
+DEF_FUNCTION_TYPE (ULONG, VOID)
+DEF_FUNCTION_TYPE (PVOID, BND)
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 888f5ad..ac79231 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -81,6 +81,8 @@ along with GCC; see the file COPYING3. If not see
#include context.h
#include pass_manager.h
#include target-globals.h
+#include tree-chkp.h
+#include rtl-chkp.h
static rtx legitimize_dllimport_symbol (rtx, bool);
static rtx legitimize_pe_coff_extern_decl (rtx, bool);
@@ -28723,6 +28725,19 @@ enum ix86_builtins
IX86_BUILTIN_XABORT,
IX86_BUILTIN_XTEST,
+ /* MPX */
+ IX86_BUILTIN_BNDMK,
+ IX86_BUILTIN_BNDSTX,
+ IX86_BUILTIN_BNDLDX,
+ IX86_BUILTIN_BNDCL,
+ IX86_BUILTIN_BNDCU,
+ IX86_BUILTIN_BNDRET,
+ IX86_BUILTIN_BNDNARROW,
+ IX86_BUILTIN_BNDINT,
+ IX86_BUILTIN_SIZEOF,
+ IX86_BUILTIN_BNDLOWER,
+ IX86_BUILTIN_BNDUPPER,
+
/* BMI instructions. */
IX86_BUILTIN_BEXTR32,
IX86_BUILTIN_BEXTR64,
@@ -28796,6 +28811,8 @@ struct builtin_isa {
enum ix86_builtin_func_type tcode; /* type to use in the declaration */
HOST_WIDE_INT isa; /* isa_flags this builtin is defined for */
bool const_p;/* true if the declaration is
constant */
+ bool leaf_p; /* true if the declaration has leaf attribute
*/
+ bool nothrow_p; /* true if the declaration has nothrow
attribute */
bool
