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.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 layoutoptimizations for GCC, and I am again kindly requesting for a review ofthe 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 unknowngimple 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-analysisHaving 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 orcomments, 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 eliminateddoing 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 Iwanted 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. Ialso 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 PHIsfor (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/operationkind 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.
