[PATCH, i386, MPX, 1/X] Support of Intel MPX ISA. 1/2 Bound type and modes

[Jeff Law]

On 09/17/13 02:18, Ilya Enkovich wrote:
> Hi,
>
> Here is a patch introducing new type and mode for bounds. It is a part of MPX ISA support patch (http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01094.html).
>
> Bootstrapped and tested on linux-x86_64. Is it OK for trunk?
>
> Thanks,
> Ilya
> --
>
> gcc/
>
> 2013-09-16  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	* mode-classes.def (MODE_BOUND): New.
> 	* tree.def (BOUND_TYPE): New.
> 	* genmodes.c (complete_mode): Support MODE_BOUND.
> 	(BOUND_MODE): New.
> 	(make_bound_mode): New.
> 	* machmode.h (BOUND_MODE_P): New.
> 	* stor-layout.c (int_mode_for_mode): Support MODE_BOUND.
> 	(layout_type): Support BOUND_TYPE.
> 	* tree-pretty-print.c (dump_generic_node): Support BOUND_TYPE.
> 	* tree.c (build_int_cst_wide): Support BOUND_TYPE.
> 	(type_contains_placeholder_1): Likewise.
> 	* tree.h (BOUND_TYPE_P): New.
> 	* varasm.c (output_constant): Support BOUND_TYPE.
> 	* doc/rtl.texi (MODE_BOUND): New.
Mostly OK.  Just a few minor things that should be fixed or at least 
clarified.




>
> diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
> index 1d62223..02b1214 100644
> --- a/gcc/doc/rtl.texi
> +++ b/gcc/doc/rtl.texi
> @@ -1382,6 +1382,10 @@ any @code{CC_MODE} modes listed in the @file{@var{machine}-modes.def}.
>   @xref{Jump Patterns},
>   also see @ref{Condition Code}.
>
> +@findex MODE_BOUND
> +@item MODE_BOUND
> +Bound modes class.  Used to represent values of pointer bounds.
I can't help but feel more is needed here -- without going into the 
details of the MPX implementation we ought to say something about how 
these differ from the more normal integer modes.  Drawing from the brief 
discussion between Richard & myself earlier today should give some ideas 
on how to improve this.



I'd probably use MODE_POINTER_BOUNDS which is a bit more descriptive. 
We wouldn't want someone to (for example) think this stuff relates to 
array bounds.  Obviously a change to MODE_POINTER_BOUNDS would propagate 
into other places where you use "BOUND" without a "POINTER" 
qualification, such as "BOUND_MODE_P" which we'd change to 
POINTER_BOUNDS_MODE_P.

> diff --git a/gcc/mode-classes.def b/gcc/mode-classes.def
> index 7207ef7..c5ea215 100644
> --- a/gcc/mode-classes.def
> +++ b/gcc/mode-classes.def
> @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3.  If not see
>     DEF_MODE_CLASS (MODE_RANDOM),		/* other */			   \
>     DEF_MODE_CLASS (MODE_CC),		/* condition code in a register */ \
>     DEF_MODE_CLASS (MODE_INT),		/* integer */			   \
> +  DEF_MODE_CLASS (MODE_BOUND),            /* bounds */                     \
>     DEF_MODE_CLASS (MODE_PARTIAL_INT),	/* integer with padding bits */    \
>     DEF_MODE_CLASS (MODE_FRACT),		/* signed fractional number */	   \
>     DEF_MODE_CLASS (MODE_UFRACT),		/* unsigned fractional number */   \
Does genmodes do the right thing WRT MAX_INT_MODE and MIN_INT_MODE?

I'd be more comfortable if MODE_POINTER_BOUNDS wasn't sitting between 
MODE_INT and MODE_PARTIAL_INT.  I'm not aware of code that iterates over 
these things that would get confused, but ISTM putting 
MODE_POINTER_BOUNDS after MODE_PARTIAL_INT is marginally safer.



> diff --git a/gcc/tree.c b/gcc/tree.c
> index b469b97..bbbe16e 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -1197,6 +1197,7 @@ build_int_cst_wide (tree type, unsigned HOST_WIDE_INT low, HOST_WIDE_INT hi)
>
>       case INTEGER_TYPE:
>       case OFFSET_TYPE:
> +    case BOUND_TYPE:
>         if (TYPE_UNSIGNED (type))
>   	{
>   	  /* Cache 0..N */
So here you're effectively treading POINTER_BOUNDS_TYPE like an integer. 
  I'm guessing there's a number of flags that may not be relevant for 
your type and which you might want to repurpose (again, I haven't looked 
at the entire patchset).  If so, you want to be real careful here since 
you'll be looking at (for example) TYPE_UNSIGNED which may not have any 
real meaning for POINTER_BOUNDS_TYPE.

Overall, it seems fairly reasonable -- the biggest concern of mine is in 
the last comment.  Are you going to be repurposing various flag bits in 
the type?  If so, then we have to be more careful in code like above.


Jeff