Allow for selection of diversly contextualized main functions in Bir #184

mdurero · 2022-09-12T08:44:14Z

Bir_interface currently overwrites silently MPP-declared main function by a "context_function", adding reset of input variables and valuation of constants and conditional statements to verify according to m_spec file used. If no m_spec is used, a reset of the whole input table is added.

This results in unwanted behaviour for some backends and obfuscates which part of the statements originates from MPP files or M_spec files.

Target for this PR as for now is :

Bir module public interface should provide access to all three of bare main_function, main_function with context from a m_spec (context_function) and context_function providing a reset of the input table (needed for the Mlang built-in interpreter)
Bir.program function map should stay untouched by Bir_interface, reflecting the computation as described in M and MPP, that's why statements for context_function and Co. should be stored in specific fields of the program record.

…n and context_function

…ction

The context_function now don't reset input table. Most parts of inner Mlang making use of context_function or Bir.main_statements are now using context_function_with_reset or Bir.main_statements_with_reset as they are linked to the interpreter.

…o_context

…and store them as their own mpp_function field

Keryan-dev · 2022-09-13T08:47:39Z

src/mlang/backend_ir/bir.mli

@@ -61,6 +61,8 @@ type program = {
  mpp_functions : mpp_function FunctionMap.t;
  rules_and_verifs : rule_or_verif ROVMap.t;
  main_function : function_name;
+  context_function : mpp_function;
+  context_with_reset_function : mpp_function;


I'd rather have those out of the function map, since it corresponds to nothing in the mpp. Maybe a option would be to put the statements lists out in the record (or a context subrecord) with names such as e.g. undefined_inputs_init constant_inputs_init and adhoc_spec_tests. Then make sure the proper statements list are build by function below and that the rest of the code refer to those functions instead of tapping into the program record.

Right now they are out of the function map, encapsulated in a type mpp_fuction, but outside.
They could be directly put as statements lists corresponding to the same function (that's to say a function built from the context you describe and the main function).
Or as you suggest, we can store the components in a record and build them on the fly. Better for modularity, a bit scary for the OIR as the further away I get from the initial state, the more it will require a rework.

I wouldn't worry for OIR as long as you input the same date as before (and even then...)

The point with OIR is: it transforms Bir.program to Oir.program and back, this could be solved easily by adding relevant information to Oir.program.
But I don't know yet if the context part of the statements were included in the optimisation. I think so, because it was added to the function map and indistinguishable from the original statements of the program. If so, my work will have changed the optimisation behaviour.

Optimisations already lose information by burying it in its output code. If you give the same input (with context IIRC), then you'd have the same output, losing context information in the process, sure, but its output should not change.

If I keep this behaviour, the OIR will effectively bury the potential context in the mpp_functions map of the Bir.program. I feel like this would be an unwanted behaviour as part of the point of having the context isolated in its own field is to keep the ability to choose in the backends if we want to use a contextualized main function or not.

So my position would be that OIR should keep either keep intact the context statements (but then they won't be optimized), or the choice to have them should be made before OIR call, as far as I can tell it's not trivial with the current Driver.

I'm inclined to modify OIR.program to pass context information transparently as the context is not the most useful in the cases we want to optimise the generated code (like C for batch processing).

Let it drown, without the context some passes might have the wrong behaviour, and you don't want to put your nose in there for this PR.

I agree this is unsatisfactory but it's not worth spending much time on this for now. We'll discuss this in more details later.

Well, we must ignore my last commit c79a4ce then.

Let it drown,

Well, now we can't, because optimisation made it back to master and so this PR breaks it as is, so I'm trying to resolve it while staying close of the old behaviour.

…t list fields

…tion are already added by Bir_interface.adapt_program_to_function

src/mlang/test_framework/test_interpreter.ml

…nctions map is now always agnostic, context being stored outside it.

….program to the Oir output without change

…tial Bir.program to the Oir output without change" This reverts commit c79a4ce.

…function-selection-in-bir

src/mlang/optimizing_ir/bir_to_oir.ml

denismerigoux

Congrats @MdrditArthur ! The code is fine but lacks some comments. You can put comments with the (* .. *) syntax, or (** .. *) for top-level comments that will be collected by ocamldoc to generate https://mlanguage.github.io/mlang/mlang/index.html

src/mlang/test_framework/test_interpreter.ml

denismerigoux · 2022-09-27T08:33:10Z

src/mlang/backend_ir/bir.mli

@@ -57,10 +57,17 @@ type mpp_function = { mppf_stmts : stmt list; mppf_is_verif : bool }

 module FunctionMap : Map.S with type key = function_name

+type program_context = {


I'm ok with the change but you should put the rationale about why we need this context as a documentation comment here (you can start from your PR description which is quite informative).

Documentation comment proposed in commit b8e0f56.

denismerigoux · 2022-09-27T08:35:05Z

src/mlang/backend_ir/bir_interpreter.ml

@@ -752,7 +752,9 @@ module Make (N : Bir_number.NumberInterface) = struct
      (code_loc_start_value : int) : ctx =
    try
      let ctx =
-        evaluate_stmts p ctx (Bir.main_statements p) [] code_loc_start_value
+        evaluate_stmts p ctx
+          (Bir.main_statements_with_reset p)


Here a little comment about why we reset would be good

…-function-selection-in-bir

…_tgv_init and add comment explaining why BIR interpreter needs the addition of these initialisation statements

mdurero added 5 commits September 12, 2022 10:21

Expand Bir.program and Bir interface to expose both bare main_functio…

41235c5

…n and context_function

Add context_with_reset_function in Bir.program similar to context_fun…

bf82d9d

…ction

Swap behaviour of Bir.main_statements and its first derivative with/n…

d967caf

…o_context

Remove the Bir-built context functions from the program function map …

095ce46

…and store them as their own mpp_function field

Keryan-dev reviewed Sep 13, 2022

View reviewed changes

mdurero added 2 commits September 15, 2022 17:32

Replace context functions by an optional context record of 3 statemen…

340ce1a

…t list fields

Remove Test_interpreter.add_test_conds_to_combined_program, the condi…

7eabd5b

…tion are already added by Bir_interface.adapt_program_to_function

mdurero commented Sep 16, 2022

View reviewed changes

src/mlang/test_framework/test_interpreter.ml Show resolved Hide resolved

mdurero added 3 commits September 16, 2022 14:39

Remove Bir_interface.context_agnostic_mpp_functions as program mpp_fu…

5eaac4f

…nctions map is now always agnostic, context being stored outside it.

Add context statements in Oir.program to pass it from the initial Bir…

c79a4ce

….program to the Oir output without change

Revert "Add context statements in Oir.program to pass it from the ini…

f596448

…tial Bir.program to the Oir output without change" This reverts commit c79a4ce.

mdurero marked this pull request as ready for review September 16, 2022 14:03

Merge branch 'master' into main-function-selection-in-bir

c0204da

mdurero marked this pull request as draft September 19, 2022 14:55

mdurero added 2 commits September 20, 2022 15:04

Transmit context statements transparently through Oir.program

f52cc2f

Merge remote-tracking branch 'origin/master' at release 98 into main-…

adbdfa5

…function-selection-in-bir

mdurero marked this pull request as ready for review September 20, 2022 16:20

mdurero mentioned this pull request Sep 21, 2022

OCaml backend #148

Draft

mdurero added a commit that referenced this pull request Sep 21, 2022

Adapt to Bir changes in PR #184

b4eb0ef

Keryan-dev reviewed Sep 21, 2022

View reviewed changes

src/mlang/optimizing_ir/bir_to_oir.ml Outdated Show resolved Hide resolved

mdurero added 3 commits September 21, 2022 15:45

Delete outdated comment

5b66e73

Merge branch 'master' at rev100 into main-function-selection-in-bir

5bcebcc

Merge branch 'master' at release 102 into main-function-selection-in-bir

d9bf2c9

mdurero requested a review from denismerigoux September 27, 2022 06:40

denismerigoux approved these changes Sep 27, 2022

View reviewed changes

mdurero added 3 commits September 30, 2022 11:22

Merge remote-tracking branch 'origin/master' at release 103 into main…

29db93e

…-function-selection-in-bir

Rename main_statements_with_reset to main_statements_with_context_and…

5168b57

…_tgv_init and add comment explaining why BIR interpreter needs the addition of these initialisation statements

Documentation comment on program context

b8e0f56

denismerigoux merged commit 934aac9 into master Oct 3, 2022

mdurero added a commit that referenced this pull request Mar 30, 2023

Adapt to Bir changes in PR #184

203b207

mdurero added a commit that referenced this pull request Jun 30, 2023

Adapt to Bir changes in PR #184

a9072f8

mdurero added a commit that referenced this pull request Jul 6, 2023

Adapt to Bir changes in PR #184

1f0e4a0

denismerigoux deleted the main-function-selection-in-bir branch April 2, 2024 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for selection of diversly contextualized main functions in Bir #184

Allow for selection of diversly contextualized main functions in Bir #184

mdurero commented Sep 12, 2022

Keryan-dev Sep 13, 2022

mdurero Sep 13, 2022 •

edited

Loading

Keryan-dev Sep 13, 2022

mdurero Sep 14, 2022

Keryan-dev Sep 14, 2022

mdurero Sep 16, 2022

Keryan-dev Sep 16, 2022

mdurero Sep 16, 2022 •

edited

Loading

mdurero Sep 19, 2022

denismerigoux left a comment

denismerigoux Sep 27, 2022

mdurero Sep 30, 2022

denismerigoux Sep 27, 2022

		@@ -57,10 +57,17 @@ type mpp_function = { mppf_stmts : stmt list; mppf_is_verif : bool }

		module FunctionMap : Map.S with type key = function_name

		type program_context = {

Allow for selection of diversly contextualized main functions in Bir #184

Allow for selection of diversly contextualized main functions in Bir #184

Conversation

mdurero commented Sep 12, 2022

Choose a reason for hiding this comment

mdurero Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdurero Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denismerigoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdurero Sep 13, 2022 •

edited

Loading

mdurero Sep 16, 2022 •

edited

Loading