On Tue, Nov 3, 2020 at 5:21 PM Erick Ochoa
<erick.oc...@theobroma-systems.com> wrote:
>
> 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.
Yes, I remember the discussion. Still GIMPLE is "flat", there's
no need to have a DFS walk at all, if then you need it for
access paths on memory reference trees but those are
linear (the TREE_OPERAND (.., 0) chain).
The way the code is structured makes it really hard to spot
what is wrong about it and nail down the fact, leading to
the simplification I expect is possible :/
> 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.
As long as IPA-SRAs doing is handled conservatively and
having IPA SRA enabled does not disable all of DFE out of
caution you can probably deal with this later.
> >
> > 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.
In GIMPLE you could replace all pointer types of SSA names with void *
and that would not make the IL invalid or change its semantics. You
won't ever see a cast from one pointer type to another in GIMPLE for
example (because such cast would be pointless). That means
expecting a type that appears in source to still appear in GIMPLE
is wrong.
As said it's hard to see how the particular DFS walk worker affects
the state, I just spotted that you do type queries on operands of
pointer arithmetic. You can simplify this by assuming the type
is void * and the outcome must not change the validity of your
analysis.
> >
> > 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).
Fair enough for the purpose you lay out but I think we agreed that
using sth like type escape is not what we want to have.
> 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.
> >
