On Wed, 24 Jul 2013, Ilya Enkovich wrote:

> Here is a patch which adds support for new instructions from Intel
> Memory Protection Extensions (MPX) ISA [1]
> 
> This patch introduces bound type, modes, registers and all MPX instructions.
> 
> Control transfer instructions are modified to support 'bnd' prefix
> used by Intel MPX.
> 
> Bootstrap and checked on x86_64-linux. There is one issue [2] with
> length attribute computation which leads to several fails in make
> check. Currently I assume it is not a bug in this patch. But in case
> it is a patch problem I have a version which uses temp attribute for
> length computation as a workaround (resulting in clean make check).
> 
> Does it look good for trunk?
This patch has no documentation (no changes to .texi files to document the 
new features added for users compiling code in C or other languages) and 
no testcases, so I have no idea what it is supposed to do at the 
user-visible level and I don't think it's suitable to be considered for 
trunk without either adding documentation and testcases to it, or posting 
subsequent patches in the series that actually add the user-visible 
features (and include documentation and testcases for them) that this 
patch is meant to enable.

(If this is the start of a patch series, you probably want to start with a 
0/N mail from that series, explaining the processor feature in high-level 
terms, how you intend that feature to be used from C code, the internal 
datastructures used inside the compiler to correspond to the uses of that 
feature from C, and the rationale for the choices you made.  Then this 
present patch might be 1/N, followed by a series of other patches adding 
other infrastructure or user-visible features.)

Your new function mode_for_bound lacks a comment explaining its semantics 
(those of the arguments and return value).

The definition of the new tree code BOUND_TYPE needs a substantial comment 
explaining the full, machine-independent semantics of such a tree type and 
of whatever tree fields are used in such a type - it's not a standard C 
concept.

-- 
Joseph S. Myers
jos...@codesourcery.com

Reply via email to