Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup primitive #232

Merged
merged 75 commits into from
Jul 11, 2024
Merged

Cleanup primitive #232

merged 75 commits into from
Jul 11, 2024

Conversation

david-michel1
Copy link
Collaborator

No description provided.

Copy link
Contributor

@denismerigoux denismerigoux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo @david-michel1, le refactoring avance bien ! Cette PR supprime plein de vieux code inutile, j'approuve. Petit bémol sur le changement de l'interpéteur d'un code fonctionnel sans effet de bord à un fonctionnement avec TGV mutable. C'est plus proche de la réalité mais ça rend plus difficile de lire une sémantique propre pour ton M étendu et du coup ça rend plus difficile la création d'analyses statiques formelles par dessus (ce qui est quand même notre idéal lointain).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Il faut renommer ce fichier mir_interpreter.ml et le mettre dans le dossier mir/ non ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

La hiérarchie des passes n'est pas encore fixée. Mais l'interpréteur va sans doute opérer sur une structure Mir. D'ailleurs, Com va peut-être devenir le nouveau Mir.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Le répertoire backend_ir a été effacé. Le code de l'interpréteur est maintenant dans m_ir.

Comment on lines 30 to 47
type print_ctx = { mutable indent : int; mutable is_newline : bool }

type ctx = {
ctx_local_vars : value Pos.marked Mir.LocalVariableMap.t;
ctx_vars : var_value Bir.VariableMap.t;
ctx_it : Mir.variable IntMap.t;
ctx_tgv : value Array.t;
ctx_tmps : value Array.t;
mutable ctx_tmps_org : int;
ctx_it : Mir.Var.t Array.t;
mutable ctx_it_org : int;
ctx_pr_out : print_ctx;
ctx_pr_err : print_ctx;
ctx_anos : (Mir.error * string option) list;
ctx_old_anos : StrSet.t;
ctx_nb_anos : int;
ctx_nb_discos : int;
ctx_nb_infos : int;
ctx_nb_bloquantes : int;
ctx_finalized_anos : (Mir.error * string option) list;
ctx_exported_anos : (Mir.error * string option) list;
mutable ctx_anos : (Com.Error.t * string option) list;
mutable ctx_old_anos : StrSet.t;
mutable ctx_nb_anos : int;
mutable ctx_nb_discos : int;
mutable ctx_nb_infos : int;
mutable ctx_nb_bloquantes : int;
mutable ctx_finalized_anos : (Com.Error.t * string option) list;
mutable ctx_exported_anos : (Com.Error.t * string option) list;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alors je ne suis pas vraiment favorable à avoir des champs mutable dans le contexte de l'interprète. Jusqu'à présent, l'interprète était purement fonctionnel et cela permet d'en lire le code sans avoir à se préoccuper des effets de bords. Est-ce que c'est pour un gain de performance de l'interpréteur ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est un essai qui ne me satisfait pas pleinement. En fait, je voulais avoir un interpréteur plus rapide (et il l'est). Mais il perd sa fonction de référence sémantique, ce qui est inacceptable. Je vais donc peut-être me retrouver avec un interpréteur de référence et un interpréteur optimisé.

Le code est dans un état intermédiaire. La pull-request porte mal son nom.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comme la sémantique est impérative, avoir un interpréteur impératif facilite le codage des backends: les différentes versions se ressemblent beaucoup plus. Je vais laisser l'interpréteur comme ça pour l'instant.

let rec evaluate_stmt (canBlock : bool) (p : Bir.program) (ctx : ctx)
(stmt : Bir.stmt) (loc : code_location) =
let set_var_value (p : Mir.program) (ctx : ctx) (var : Mir.Var.t)
(vexpr : Mir.expression Pos.marked) : unit =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Voilà, je pense que c'est un problème que ce genre de fonctions retourne unit et pas ctx...

global_description = input_var.Mast.input_description;
global_typ = Option.map Pos.unmark input_var.Mast.input_typ;
}
Mir.Var.new_tgv ~name:input_var.Mast.input_name ~is_table:None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_tgv créé un TGV ou une variable de TGV ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comme la fonction est dans le module Var, c'est forcément une variable TGV, que je vais sans doute renommer "variable globale".

| None -> 0
in
let prog_targets =
let rec aux nbIt = function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qu'est ce que aux fait ? C'est pas clair en lisant le code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aux fait des trucs auxiliaires... Elle changera de nom si ce qu'elle calcule sera conservé.

| "supzero" -> check_func 1
| "present" ->
| _ -> Err.multimax_require_two_args expr_pos)
| Com.SumFunc -> check_func (-1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 arguments ? C'est comme ça que tu encodes "un nombre arbitraire d'arguments" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pour l'instant... Je n'avais pas envie de changer toute la structure de la fonction, et de créer un type juste pour ça.

@@ -2186,8 +2338,9 @@ let proceed (p : Mast.program) : program =
| Mast.Rule r -> check_rule r prog
| Mast.Verification v -> check_verif v prog)
prog source_file)
(empty_program p app) p
(empty_program p app main_target)
p
in
prog |> complete_rdom_decls |> complete_vdom_decls |> convert_rules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alors ici ce serait vachement bien d'avoir un commentaire à chaque étape qui résume à grands traits ce que cette étape fait !

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

La moitié de Check_validity va déménager dans un autre fichier, vu qu'il ne s'agit plus de valider la cohérence du code M mais de le transformer en code générique.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Du coup Com c'est la partie de l'AST commune entre Mast et Mir ? Si oui c'est malin, j'approuve.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est commun le temps de rendre les structures communes (Mast, Mir, Bir). Quand ça sera fait, elle changera de nom puisque les choses communes auront disparu.

Copy link
Contributor

@denismerigoux denismerigoux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this!

@denismerigoux denismerigoux merged commit e15393b into master Jul 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants