Skip to content

Basic unique Extension#138

Closed
riaqn wants to merge 93 commits intoocaml-flambda:mainfrom
riaqn:unique-base
Closed

Basic unique Extension#138
riaqn wants to merge 93 commits intoocaml-flambda:mainfrom
riaqn:unique-base

Conversation

@riaqn
Copy link
Copy Markdown
Contributor

@riaqn riaqn commented Mar 8, 2023

This PR is a simplified version of #116 where overwrite ({overwrite_ x with ...}) is removed. The overwrite feature requires changes in the middle-end and back-end which is easily unsafe if not done right. This PR doesn't include overwrite and should be easier to review and merge. Moreover, most of the race-freedom guarantee we aim to provide doesn't rely on overwriting anyway.

This PR still touches the middle and back-end because

  • Most are the renaming from alloc_mode to locality_mode. Now alloc_mode contains two axes: locality and uniqueness.
  • The uniqueness axis is passed through, because they might be needed even in middle and back-end.
    Currently we only use it to disable constant-lifting for unique values in multiple stages in the pipeline.

@riaqn riaqn mentioned this pull request Mar 8, 2023
@riaqn riaqn requested a review from lpw25 March 9, 2023 17:12
@riaqn riaqn force-pushed the unique-base branch 2 times, most recently from 5371521 to 7fa33df Compare March 15, 2023 17:37
@riaqn riaqn force-pushed the unique-base branch 2 times, most recently from f55ab8b to d7c0ded Compare March 28, 2023 09:31
Copy link
Copy Markdown
Contributor

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

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

I read through most of the changes to the back and middle-end, but I actually think it would be better to take those out since they can't really be tested in this PR. Better to have a PR that obviously doesn't change compilation, and then later change compilation when we have operations that require those changes.

@riaqn
Copy link
Copy Markdown
Contributor Author

riaqn commented Apr 17, 2023

This PR is now rebased, and stripped off most middle-end and back-end changes.

@riaqn riaqn mentioned this pull request Apr 21, 2023
7 tasks
@riaqn riaqn requested a review from lpw25 May 2, 2023 10:50
@riaqn
Copy link
Copy Markdown
Contributor Author

riaqn commented Aug 7, 2023

After refactoring, most of issues are addressed. . However, because of the big changes, I'm not sure if some of those are addressed in the ideal way. Therefore, I left those comments open. Could you take another look and let me know @lpw25

Copy link
Copy Markdown
Contributor

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

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

Some comments on recent changes. I'm also going to push some changes directly for the parts of uniqueness_analysis.ml I haven't yet reviewed.

Copy link
Copy Markdown
Contributor

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

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

Some final suggestions, but I think this is probably good to go.

@lpw25
Copy link
Copy Markdown
Contributor

lpw25 commented Aug 18, 2023

This has now been merged in the other repo.

@lpw25 lpw25 closed this Aug 18, 2023
@lpw25 lpw25 mentioned this pull request Aug 18, 2023
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.

2 participants