This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, i386, MPX 1/X] Support of Intel MPX ISA


On Thu, 25 Jul 2013, Ilya Enkovich wrote:

> > Usually also new instructions have a -m option to enable them, but you
> > don't have that here either.  I realise the instructions are NOPs on
> > processors not supporting them (all processors not supporting them?), but
> > given that the availability of the instructions must at some point affect
> > either the semantics of RTL containing checks, or what RTL the compiler
> > can generate code for, I'd think you should have such an option anyway.
> 
> We had this option initially but removed it because it was useless.
> You get nothing when pass -mmpx and you always have to to pass it when
> pass -fmpx. Introduction of that option just makes you to always use
> pair '-fmpx -mmpx' which is not convenient.

It still seems wrong for -f options to enable new architecture-specific 
instructions; that's the job of -m options (including -march= options that 
imply -mmpx, of course).

> > Thanks.  You also need to post the implementation design in terms of
> > semantics of machine-independent tree/GIMPLE/RTL additions (which of
> > course should be added to the relevant .texi files).
> 
> Could you please be more specific regarding .texi files? In my
> implementation I add
>  - new type
>  - new modes
>  - new options
>  - new builtins
>  - new target hooks
> I know where hooks and options should be documented. But what about
> others? For now I saw only description in comments.

Built-in functions are documented in extend.texi.  Some aspects of tree 
and GIMPLE are documented in generic.texi and gimple.texi.  The various 
individual machine modes, and classes of modes, are documented in 
rtl.texi.  If there are new RTL insn patterns, those are also documented 
in rtl.texi.

Configure options are documented in install.texi.  (Arguably, if 
MPX-enabled versions of runtime libraries are built they should be *in 
addition to* normal versions - that is, extra multilibs, as the right 
choice may not be the same for all programs built with a given compiler.)

> tree-mpx.c files has a description in a header. Does it look close to
> implementation design you are talking about?

It seems useful - but to the extent that it refers specifically to MPX (or 
the file is so-named to refer to MPX) it's at the wrong level.  It should 
describe things in terms of the architecture-independent GIMPLE 
representation of the bounds checks (with any architecture-dependencies 
needed at this point, controlled by hooks, being subsidiary to that).  
Then, I suppose there would be lowering to some architecture-specific 
form, whether as a GIMPLE pass or at the time of RTL expansion.

> I'll look at -fbounds-check option.
> I'll also add some additional description and comparison on GCC Wiki.

Thanks.  I'd imagine the desired state as:

* generic infrastructure for determining bounds for objects, tracking them 
through pointer arithmetic, optimizing away redundant bounds checks, etc., 
which could be shared between multiple different forms of bounds checking 
supported in GCC; and

* implementations of the different bounds-checking strategies (MPX, ASAN, 
etc.), based on the information / internal representation built up by 
common infrastructure, and using architecture-specific features where 
appropriate.

Maybe that's not realistic given the present state of GCC or the 
differences between the different bounds-checking approaches.  But you're 
the one working on the bounds checking - you need to produce the 
explanation of what there is or is not in common between the different 
approaches that justifies the choice of what to share or not to share in 
the implementations.  And you also need to write the user documentation 
for invoke.texi that allows a user to understand the trade-offs at the 
level of their source code, the performance of the generated code and what 
conditions will or will not be detected, so users can make sensible 
choices of which bounds-checking options to use based on their 
requirements.

> > Naming new built-in functions __bnd_* seems unfortunate.  We already have
> > __builtin_*, __sync_* and __atomic_* as built-in function naming
> > conventions (with consequent checks in various places based on function
> > name) - do we really need yet another such prefix?
> >
> 
> Both __bnd_ and __builtin___bnd variants are available. The reason for
> builtins name is simple - Intel docs. It is always nice to have
> compatibility. Actually Intel declares single underscore in the
> beginning of the name but I could not make it work on C++ with a
> single underscore. Is there a way to fix it?

The normal way to have compatibility in such cases is a header that 
#defines the other-compiler names to the GCC names.

At least is_builtin_name would need updating for any new built-in function 
name prefix - but as noted, I don't advise adding such new prefixes at 
all.

-- 
Joseph S. Myers
joseph@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]