[gen] Refactor communication edge representation#1871
Conversation
15087d9 to
18b0b44
Compare
TiberiuBucur
left a comment
There was a problem hiding this comment.
A few minor comments / requests for clarifications :)
| let r = fold_ie wildcard (fun ie -> f (Fr ie)) r in | ||
| let r = fold_ie wildcard (fun ie -> f (Ws ie)) r in | ||
| let r = fold_com (fun com r -> fold_ie wildcard (fun ie -> f (Communication (com,ie))) r) r in |
There was a problem hiding this comment.
Is it guaranteed that the f parameter will return the same result when applied to the same operators no matter the order? Because the old version did Rf -> Fr -> Ws whereas fold_com does the opposite.
There was a problem hiding this comment.
I change the order back to Rf -> Fr -> Ws(Co).
There is no guarantee on the same behaviour on different order. However the order does not matter in the the way we use all the fold_ functions. They are used to build a look-up table.
There was a problem hiding this comment.
I see. I don't have an issue either way if you think it doesn't make a difference, but in that case I would personally keep the definition of fold_com in the original order (Rf as the outermost parameter, Co as the innermost) for the sake of consistency.
| | {edge=(Rf Ext|Fr Ext|Ws Ext); a2=Some a; _}:: | ||
| ({edge=(Rf Ext|Fr Ext|Ws Ext);a1=Some _; _}::_ as es) -> | ||
| | {edge=Communication (_,Ext); a2=Some a; _}:: | ||
| ({edge=Communication (_,Ext);a1=Some _; _}::_ as es) -> |
There was a problem hiding this comment.
I understand this behaviour existed before this patch, but why do this function and the one below only concern themselves with external communication edges?
There was a problem hiding this comment.
I do not have a definitive answer but I believe the function count_a is used to heuristically to generate file name.
Unify `Rf`, `Fr`, and `Ws` edge constructors behind `Communication (com, ie)` and rename Code.com constructors to `Rf`, `Fr`, and `Co`. Update edge folding, parsing, naming, relaxation handling, and generation code to use the unified communication representation while preserving existing printed syntax.
18b0b44 to
7abb20e
Compare
|
Sorry I’m still not convinced we want Com to be a constructor in edge.ml. Can you please explain what is it that you would like to achieve with this PR? Let’s focus on that first and then I can help with the how. |
This is purely code improve for now but the main goal here is to implement Also there were two datatypes that were the same thing but used in two separate places in the code.
The two datatypes were the same, so the best way is to fold the second datatype for communication edge into |
Exp-obs is not the only derived relation we need for aarch64.cat. We have many more, for example dob, are you suggesting we do that for data dependencies as well? I’m not convinced this PR is the right solution to the problem. I think we need to come up with a way to define new symbols which group together multiple relaxations. This would work for Exps-obs as well as other relations in the cat file. This would be more syntactic sugar for the use of diy7 and not a fundamental relaxation. What I mean is that the user would write ExpObs and diy7 would expand it to Rfe but the gen code will know nothing else about ExpObs.
This part of the change seems sensible. |
I totally agree here. This is similar to issue #1784. I think we should treat There is nothing to stop us to remove the use of wildcard completely, in fact I already have a local PR for that. Yet it breaks some test cases on
The change here is purely for the second part, hence I decide to put forwards first, which can be independent from how we want to implement |
If the plan is to do this thing for all kinds of aarch64 dependencies, do you think diy can use the cat file as some sort of ground truth for what "primitive edges" (coe, rfe, fre) a dependency is made of? It would help with future changes to the cat file not requiring updates to diy. |
FWIW, this is precisely what |
@TiberiuBucur @fsestini That is a good comment. I think it is certainly possible to gradually bring in some cat language, for example treating them as macros first, as the ground true into I think some of this comments are much more related to #1784 , if you @TiberiuBucur are interested, we can collaborate ! |
For the record, I'm not proposing a solution here. Just wanted to point out that if we do decide to involve the cat model for this sort of feature, |
Unify constructors
Rf,Fr, andWsinrelax.mland the similar constructorsCRf,CFrandCWsincode.ml. The former has been wrapped behindCommunication (com, ie)and rename the latter constructors toRf,Fr, andCo. Update edge folding, parsing, naming, relaxation handling, and generation code to use the unified communication representation while preserving existing printed syntax.