Thanks for the review Richard I'll address what I can. I also provide
maybe some hindsight into some of the design decisions here. I'm not
trying to be defensive just hoping to illuminate why some decisions were
made and how some criticisms may fail to really address the reason why
these designs were made.
On 03/11/2020 15:58, Richard Biener wrote:
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.
Thanks I didn't know this!
You expose way too many user-visible command-line options.
Yes, I can certainly remove some debugging flags.
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.
There are two points being made here:
1. Use the available walkers
2. Changing gimple would imply changes to your code
First:
I did tried using the available walkers in the past, and the walk_tree
function does not contain a post-order callback. We really need to
propagate information from the gimple leaf nodes back to the root, and
as a way to achieve this (and probably other things like maintaining a
stack of the nodes visited to reach the current node) we really need a
post-order callback.
We did have a conversation about this where you pointed out this pattern:
*walk_subtrees = 0;
walk_tree (.. interesting subexpressions ... );
do post-order work
In the preorder hook, but this only works with simple expressions and we
need more complicated machinery.
Furthermore, I did look into extending the current walk_tree function
with post-order callbacks but due to implementation details in the
function (tail-recursion), we both agreed that having both
tail-recursion AND a post-order hook was impossible.
Second:
I would expect any transformation that performs an analysis in an IR be
needing to at least be re-reviewed somehow when the IR is re-written. I
also don't think extending the base visitor classes would be too difficult.
That "IPA SRA is not safe" is of course not an option but hints
at a shortcoming in your safety analysis.
I looked into how to make this transformation safe with IPA-SRA in the
past. I don't think it is really that big of a short-coming. I was told
that I could look at cgraph_node->clone.param_adjustments and find out
how the parameters are being adjusted. I will certainly work on this,
however, due to exploring the feasibility of using a points-to-analysis
instead of a type-based escape analysis to drive this transformation,
making this transformation safe with IPA SRA had to be put on the back
burner.
So, overall, yes, I agree that it would be better if this transformation
worked with IPA-SRA and I also think it can be done.
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.
What exactly do you mean by this "not carry any semantics"? If we are
modifying a type, a type carries the semantics of how big the expected
type is. So I at least need to know whether the pointer arithmetic
affects a type I modified and if that's the case how big the previous
type was and how smaller it is once the fields are deleted.
Can you be more concrete and point out the specific line of code? I
think I have covered all cases here, but I just want to be really sure.
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.
We could certainly flatten the visitors into a single larger visitor.
The idea is to maintain a separation between statements, expressions,
and types. That is why there are visitors for statements, expressions
and types. And sure, each stage of the analysis might need slightly
different ways to model gimple, so there are several derived classes.
Again, this was a design decision and I'm not married to it. I will try
to refactor it to have a larger visitor.
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.
Originally I wrote these transformations with a lot of assertions to
make fuzzying easier. In my experience, I have not seen REFERENCE_TYPE,
QUAL_UNION_TYPE, nor METHOD_TYPE being generated from C code. Could we
handle these gimple types in the future? Sure! I'd be happy to
contribute such changes. However, since this optimization was written
mostly targetting C code, I would need some time to revisit the
assumptions that were made at the time and make sure that the
implementation does not break. So, instead, at the moment, if we detect
something that is not normally seen in "Gimple from C", we will abort
and exit the optimization / analysis without transforming anything.
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.
I don't want to use modref only to replace the void* "coloring"
heuristic. What I want is to use IPA-PTA to avoid using the IPA type
escape analysis, however the level of precision for IPA-PTA needed for
such things is not enough at the moment. I believe that using modref in
the IPA type escape analysis, while the infrastructure for IPA-PTA grows
is the best way to go forward. It gives me more experience writing
transformations following the style that the community likes, learn
about infrastructure available in GCC, and provides a way to test the
future implementation (i.e., we can test the IPA-PTA implementation
against the type-escape implementation).
Again, thank you very much for the review! It will be hard for me to
address some of these concerns in a timely manner. I would appreciate if
we can continue having some discussion about the design decisions I made
and whether or not they do make sense and maybe are enough to address
some concerns. Otherwise, I'll just try my best to address the concerns
listed here by changing the implementation.
Richard.
