Dead Field Elimination and Field Reordering

Dead Field Elimination and Field Reordering

[Erick Ochoa]

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.

Re: Dead Field Elimination and Field Reordering

[Richard Biener]

On Fri, Oct 30, 2020 at 6:44 PM Erick Ochoa
<erick.ochoa@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.

Re: Dead Field Elimination and Field Reordering

[Erick Ochoa]

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.ochoa@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.
> 

Re: Dead Field Elimination and Field Reordering

[Richard Biener]

On Tue, Nov 3, 2020 at 5:21 PM Erick Ochoa
<erick.ochoa@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.ochoa@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.
> >

Re: Dead Field Elimination and Field Reordering

[Erick Ochoa]

Hi Richard,

just some top-level comments before I write about anything specific:

* I have removed all non-essential flags I introduced
* I have placed the standard headers before config
* I have squashed some changes that I sent to the patches mailing list 
and make sure that the transformation bootstraps on every commit
* I will be sending another set of patches to the patch mailing list 
soon that have the changes mentioned above.

Things I will be working on the next couple of days:

* Removing std datastructures and using GCC specific ones.
* Adding support for ipa-sra.
* Flattening the visitors.

On 05/11/2020 14:10, Richard Biener wrote:
> On Tue, Nov 3, 2020 at 5:21 PM Erick Ochoa
> <erick.ochoa@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.ochoa@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 :/

Ok, I think this style difference can be solved through some 
refactoring. It will take time but I think it is possible to achieve. I 
am definitely biased in keeping the visitor style and I see many 
benefits over what you are suggesting, but again, I am willing to make 
the changes you request.

> 
>> 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.


Yes, I'm 99% sure this can be achieved easily.

> 
>>>
>>> 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.

I agree with you that changing void* does not make invalid gimple nor 
change its semantics. However, it is my understanding that this is an 
edge case and normal gimple will likely have type information preserved. 
The only evidence I can bring for this is the fact that we have been 
applying this transformation successfully in the past.

In the case that you mentioned, where the type information does not 
match what we expect, the transformation would not be applied.

Thanks again for the review, as mentioned above, I have changed the code 
to address the most concrete criticisms received so far. I will be 
submitting new patches to the patches mailing list later today and 
continue working on addressing some of the larger concerns. I welcome 
all criticisms and look forward to addressing concerns / have a 
discussion about this transformation.

> 
>>>
>>> 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.
>>>

Re: Dead Field Elimination and Field Reordering

[Erick Ochoa]

Hello,

I will be posting today some changes on the patches mailing list. I 
believe that due to some changes in the way clones are materialized, the 
transformation now supports IPA-SRA by default. I still need to test 
this more thoroughly, but initial tests show that this is no longer an 
issue.

After this, I will be removing std data structures and using specific ones.

If anyone have any comments about the transformation, please let me 
know. I am happy to answer questions.

-Erick

On 06.11.20 05:51, Erick Ochoa wrote:
> Hi Richard,
> 
> just some top-level comments before I write about anything specific:
> 
> * I have removed all non-essential flags I introduced
> * I have placed the standard headers before config
> * I have squashed some changes that I sent to the patches mailing list 
> and make sure that the transformation bootstraps on every commit
> * I will be sending another set of patches to the patch mailing list 
> soon that have the changes mentioned above.
> 
> Things I will be working on the next couple of days:
> 
> * Removing std datastructures and using GCC specific ones.
> * Adding support for ipa-sra.
> * Flattening the visitors.
> 
> On 05/11/2020 14:10, Richard Biener wrote:
>> On Tue, Nov 3, 2020 at 5:21 PM Erick Ochoa
>> <erick.ochoa@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.ochoa@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 :/
> 
> Ok, I think this style difference can be solved through some 
> refactoring. It will take time but I think it is possible to achieve. I 
> am definitely biased in keeping the visitor style and I see many 
> benefits over what you are suggesting, but again, I am willing to make 
> the changes you request.
> 
>>
>>> 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.
> 
> 
> Yes, I'm 99% sure this can be achieved easily.
> 
>>
>>>>
>>>> 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.
> 
> I agree with you that changing void* does not make invalid gimple nor 
> change its semantics. However, it is my understanding that this is an 
> edge case and normal gimple will likely have type information preserved. 
> The only evidence I can bring for this is the fact that we have been 
> applying this transformation successfully in the past.
> 
> In the case that you mentioned, where the type information does not 
> match what we expect, the transformation would not be applied.
> 
> Thanks again for the review, as mentioned above, I have changed the code 
> to address the most concrete criticisms received so far. I will be 
> submitting new patches to the patches mailing list later today and 
> continue working on addressing some of the larger concerns. I welcome 
> all criticisms and look forward to addressing concerns / have a 
> discussion about this transformation.
> 
>>
>>>>
>>>> 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.
>>>>