add `[%exclave]` support by riaqn · Pull Request #51 · ocaml-flambda/ocaml-jst

riaqn · 2022-10-14T15:34:02Z

This patch adds the support for the [%exclave] extension, which can be applied to an expression and results in early ending of the current region before the expression is evaluated. As such, it can only appear in the tail position of the region. A motivating example is as follows:

let foo (local_ x) = 
let local_ y = (complex computation on x) in 
if y then [%exclave] (local_ None)
else  [%exclave] (local_ (Some x))

where foo is supposed to be some helper function that takes arguments from parent region and returns values also in parent region. The intermediate results (y) it uses are stored in its local stack and released upon entering [%exclave]. As a result, you are not allowed to refer to y in [%exclave].

lpw25

A few changes needed but mostly looks good.

lambda/lambda.ml

lambda/simplif.ml

typing/typecore.ml

testsuite/tests/typing-local/unregion.ml

riaqn · 2022-11-03T16:48:28Z

This PR has been rebased on top of version 4.14.

riaqn · 2022-11-04T17:35:06Z

The commit 5bb4963 (after discussion with @lpw25 who discussed with @stedolan ) made it so that any function ending with [%unregion] will always return local_. This is to accommodate some code in the compiler which relies on that bit to decide if the function might allocate in parent region. This also (unexpectedly) gives the expected output for a test case that wasn't working before (and I didn't know why).

riaqn · 2022-10-24T14:08:55Z

typing/untypeast.ml

+    | Texp_unregion exp ->
+        Pexp_apply ({
+        pexp_desc =
+          Pexp_extension
+            ({ txt = "ocaml.unregion"; loc}
+            , PStr []);
+        pexp_loc = loc;
+        pexp_loc_stack = [];
+        pexp_attributes = [];
+      }, [Nolabel, sub.expr sub exp])


I recall that this file is mostly deprecated, but I wrote what I think is right anyway.

riaqn · 2022-10-24T14:12:55Z

typing/typecore.ml

+  | Pexp_apply
+      ({ pexp_desc = Pexp_extension({
+         txt = "extension.unregion" | "ocaml.unregion" | "unregion" as txt}, PStr []) },
+       [Nolabel, sbody]) ->
+      if (txt == "extension.unregion") && not (Clflags.Extension.is_enabled Local) then
+          raise (Typetexp.Error (loc, Env.empty, Local_not_enabled));


I'm merging the two very similar pattern-matching cases, and testing for one of them in the body. This might not be a good coding style, but the alternative is to have an auxiliary function that has too many arguments.

testsuite/tests/typing-local/regions.reference

riaqn · 2022-10-24T14:19:12Z

typing/rec_check.ml

+    | Texp_unregion e ->
        classify_expression env e


Please confirm this is correct

riaqn · 2022-10-24T14:19:20Z

typing/rec_check.ml

    | Texp_probe {handler} ->
      expression handler << Dereference
    | Texp_probe_is_enabled _ -> empty
+    | Texp_unregion e -> expression e


please confirm this is correct

riaqn · 2022-10-24T14:19:37Z

typing/tast_iterator.ml

      sub.expr sub e
  | Texp_probe {handler;_} -> sub.expr sub handler
  | Texp_probe_is_enabled _ -> ()
+  | Texp_unregion exp -> sub.expr sub exp


Please confirm this is correct

riaqn · 2022-10-24T14:19:52Z

typing/tast_mapper.ml

+    | Texp_unregion exp ->
+        Texp_unregion (sub.expr sub exp)


Please confirm this is correct

riaqn · 2022-10-24T14:20:08Z

typing/typecore.ml

  | Texp_letop _
  | Texp_extension_constructor _ ->
    false
+  | Texp_unregion e -> is_nonexpansive e


Please confirm this is correct

riaqn · 2022-10-24T14:20:17Z

typing/typecore.ml

+            | Texp_unregion e ->
                check e


please confirm this is correct

antalsz · 2023-01-12T18:29:22Z

We should strongly consider switching to using [@unregion] (separately, I want to make that change for [%local]); in terms of making sure code is compatible with standard OCaml, unregion is perfectly ignorable. Also, if we want to support an unregion_ keyword, then we also need to update our syntax-rewriter to support [@unregion]; that's a job for me (as long as the location information is propagated correctly), just let me know when it's necessary.

goldfirere · 2023-01-12T20:46:12Z

Yes -- let's use attributes for anything that can safely be just ignored in the upstream compiler.

riaqn · 2023-01-23T13:18:08Z

Hmm do you think I should make the change in this PR, or do it some time later? @antalsz @goldfirere

goldfirere · 2023-01-23T15:50:57Z

Antal may see things differently, but I think it's best to merge patches when they're not going to cause trouble for others. I think the infrastructure to handle e.g. [@unregion] correctly is still some time away, and so merging what you have and then quickly (this week?) changing it sounds reasonable to me.

antalsz · 2023-01-23T15:56:49Z

Because we can handle [@unregion] correctly via the refactor-based rewriter, I'd prefer making that change in ocaml-jst before we roll it out to users. Whether that means in this PR or a different PR is something I'm happy to leave in your hands, @riaqn. Also, are we planning to add syntax for it?

riaqn · 2023-02-09T16:55:50Z

I suggest that this PR to be reviewed as it is, without migrating to the new extension syntax framework, because currently the whole local extension is still living in the old world.

Sometime in the future we should migrate the whole local extension to the new syntax framework, but that would deserve a separate PR.

Maybe we can start reviewing now? @stedolan @lpw25

riaqn · 2023-02-17T12:35:48Z

typing/typecore.ml

+      (* CR zqian: this is suspicious, 
+         because vmode is not the mode of the region;
+         ideally the `position` should contain the mode 
+         of the region so we can pass it to mode_return;
+         but this might be fine because of mode-crossing? *)
+     mode_return vmode


This one needs some inspection.

riaqn · 2023-03-08T10:32:05Z

This PR relies on oxcaml/oxcaml#1131 to be merged safely.

lpw25

Looks pretty good. Should be good to go once various superficial things are fixed.

asmcomp/printcmm.ml

jane/doc/local-reference.md

typing/typecore.ml

lpw25

Small small suggestions then this is good to go

lambda/simplif.ml

testsuite/tests/typing-local/loop_regions.ml

typing/typecore.ml

jane/doc/local-reference.md

riaqn mentioned this pull request Oct 17, 2022

Add support for partial application #52

Closed

lpw25 reviewed Oct 18, 2022

View reviewed changes

riaqn force-pushed the unregion branch 2 times, most recently from fa20d57 to 2411722 Compare October 21, 2022 17:16

riaqn force-pushed the unregion branch from a03332c to b019b1f Compare November 3, 2022 16:48

riaqn force-pushed the unregion branch from 56cc796 to 98d2505 Compare November 22, 2022 12:51

riaqn commented Nov 30, 2022

View reviewed changes

riaqn force-pushed the unregion branch from 4425fd0 to 888a4c0 Compare December 6, 2022 16:10

riaqn force-pushed the unregion branch from 888a4c0 to c0179fb Compare January 12, 2023 15:29

riaqn force-pushed the unregion branch 2 times, most recently from 79e7e52 to 5f8ca38 Compare January 20, 2023 14:16

riaqn force-pushed the unregion branch from 5f8ca38 to 5208ee0 Compare February 9, 2023 17:10

riaqn force-pushed the unregion branch from 350eba7 to aae1b32 Compare February 17, 2023 10:50

riaqn commented Feb 17, 2023

View reviewed changes

riaqn force-pushed the unregion branch from 7fa665c to f911be8 Compare March 3, 2023 13:51

riaqn requested a review from lpw25 March 6, 2023 10:09

riaqn changed the title ~~add [%unregion] support~~ add [%exclave] support Mar 6, 2023

riaqn force-pushed the unregion branch from 5247047 to 1af7cb4 Compare March 6, 2023 16:04

riaqn force-pushed the unregion branch from 1af7cb4 to f596347 Compare March 15, 2023 17:46

riaqn force-pushed the unregion branch from f596347 to 773763f Compare April 4, 2023 11:01

add unregion

df78a34

riaqn added 3 commits April 4, 2023 13:36

rename unregion to exclave

6679a29

rename Ctail and Utail

9090b32

small fix

3ade286

riaqn force-pushed the unregion branch from 705606a to 3ade286 Compare April 4, 2023 12:40

fix depend

7d0666e

lpw25 reviewed Apr 11, 2023

View reviewed changes

riaqn added 3 commits April 13, 2023 17:59

revision wrt CR

54719c5

fix doc line wrap

ebd249b

Lwhile doesn't entail region

286a047

lpw25 mentioned this pull request Apr 14, 2023

Ltail for lambda and use in dissect_letrec oxcaml/oxcaml#1313

Merged

lpw25 approved these changes Apr 14, 2023

View reviewed changes

changes per PR

d9bffcb

lpw25 approved these changes Apr 17, 2023

View reviewed changes

lpw25 merged commit a2556fc into ocaml-flambda:main Apr 17, 2023

antalsz pushed a commit to antalsz/ocaml-jst that referenced this pull request May 1, 2023

Add [%exclave] support (ocaml-flambda#51)

9092201

Conversation

riaqn commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpw25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

riaqn commented Nov 3, 2022

Uh oh!

riaqn commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antalsz commented Jan 12, 2023

Uh oh!

goldfirere commented Jan 12, 2023

Uh oh!

riaqn commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goldfirere commented Jan 23, 2023

Uh oh!

antalsz commented Jan 23, 2023

Uh oh!

riaqn commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riaqn commented Mar 8, 2023

Uh oh!

lpw25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpw25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

riaqn commented Oct 14, 2022 •

edited

Loading

riaqn commented Nov 4, 2022 •

edited

Loading

riaqn commented Jan 23, 2023 •

edited

Loading

riaqn commented Feb 9, 2023 •

edited

Loading