On Fri, Oct 30, 2020 at 6:44 PM Erick Ochoa
<erick.oc...@theobroma-systems.com> wrote:
>
> Hello again,
>
> I've been working on several implementations of data layout
> optimizations for GCC, and I am again kindly requesting for a review of
> the type escape based dead field elimination and field reorg.
>
> Thanks to everyone that has helped me. The main differences between the
> previous commits have been fixing the style, adding comments explaining
> classes and families of functions, exit gracefully if we handle unknown
> gimple syntax, and added a heuristic to handle void* casts.
>
> This patchset is organized in the following way:
>
> * Adds a link-time warning if dead fields are detected
> * Allows for the dead-field elimination transformation to be applied
> * Reorganizes fields in structures.
> * Adds some documentation
> * Gracefully does not apply transformation if unknown syntax is detected.
> * Adds a heuristic to handle void* casts
>
> I have tested this transformations as extensively as I can. The way to
> trigger these transformations are:
>
> -fipa-field-reorder and -fipa-type-escape-analysis
>
> Having said that, I welcome all criticisms and will try to address those
> criticisms which I can. Please let me know if you have any questions or
> comments, I will try to answer in a timely manner.
>
> The code is in:
>
> refs/vendors/ARM/heads/arm-struct-reorg-wip
>
> Future work includes extending the current heuristic with ipa-modref
> extending the analysis to use IPA-PTA as discussed previously.
>
> Few notes:
>
> * Currently it is not safe to use -fipa-sra.
> * I added some tests which are now failing by default. This is because
> there is no way to safely determine within the test case that a layout
> has been transformed. I used to determine that a field was eliminated
> doing pointer arithmetic on the fields. And since that is not safe, the
> analysis decides not to apply the transformation. There is a way to deal
> with this (add a flag to allow the address of a field to be taken) but I
> wanted to hear other possibilities to see if there is a better option.
> * At this point we’d like to thank the again GCC community for their
> patient help so far on the mailing list and in other channels. And we
> ask for your support in terms of feedback, comments and testing.
I've only had a brief look at the branch - if you want to even have a
remote chance of making this stage1 you should break the branch
up into a proper patch series and post it with appropriate ChangeLogs
and descriptions.
First, standard includes may _not_ be included after including system.h,
in fact, they _need_ to be included from system.h - that includes
things like <vector> or <map>. There are "convenient" defines you
can use like
#define INCLUDE_SET
#include "system.h"
and system.h will do what you want. Not to say that you should
use GCCs containers and not the standard library ones.
You expose way too many user-visible command-line options.
All the stmt / tree walking "meta" code should be avoided - it
would need to be touched each time we change GIMPLE or
GENERIC. Instead use available walkers if you really need
it in such DFS-ish way.
That "IPA SRA is not safe" is of course not an option but hints
at a shortcoming in your safety analysis.
In DFE in handle_pointer_arithmetic_constants you
look at the type of an operand - that's not safe since
this type doesn't carry any semantics.
The DFE code is really hard to follow since it diverges
from GCC style which would do sth like the following
to iterate over all stmt [operands]:
FOR_EACH_BB_FN (fun, bb)
{
for (auto gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
walk PHIs
for (auto gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
walk stmts, for example via walk_gimple_stmt ()
}
and I'd expect a single visitor with a switch () over the gimple/operation
kind rather than gazillion overloads I have no idea what they exactly
visit and how.
In a later change on the branch I see sth like ABORT_IF_NOT_C
where I'm not sure what this is after - you certainly can handle
IL constructs you do not handle conservatively (REFERENCE_TYPE
is the same as POINTER_TYPE - they are exchangable for the
middle-end, METHOD_TYPE is the same as FUNCTION_TYPE,
a QUAL_UNION_TYPE is not semantically different from
a UNION_TYPE for the middle-end it only differs in layout handing.
I see you only want to replace the void * "coloring" with modref
so you'll keep using "IPA type escape analysis". I don't think
that's good to go.
Richard.
