Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@bettinaheim
Copy link
Contributor

@bettinaheim bettinaheim commented Apr 19, 2021

This PR switches to apply ref count changes only to the outer container and not the inner items unless the ref count changes are due to an assignment to a mutable variable. The reasoning is that I believe it is not possible for an inner item to ever go out of scope earlier than the container unless it is assigned to a mutable variable somewhere.

This PR makes the adjustment in #859 unnecessary (i.e. incorrect), and fixes #970. I believe it may also possibly fix some of #981 (tbd, I think there are possibly multiple issues there).

@bettinaheim bettinaheim requested a review from swernli April 19, 2021 23:14
Copy link
Contributor

@swernli swernli left a comment

Choose a reason for hiding this comment

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

The test QIR looks good and the logic is sound, so approving. We will have to rely on manual testing for runtime behavior for now.

@bettinaheim
Copy link
Contributor Author

The test QIR looks good and the logic is sound, so approving. We will have to rely on manual testing for runtime behavior for now.

Yes, I did some manual validation locally, but of course not nearly as extensive as a proper CI setup should be - let's push to get the full CI up and running.

@bettinaheim bettinaheim merged commit 9689a96 into main Apr 21, 2021
@bettinaheim bettinaheim deleted the beheim/references branch April 21, 2021 00:17
bettinaheim pushed a commit that referenced this pull request Apr 22, 2021
bettinaheim added a commit that referenced this pull request Apr 22, 2021
* Revert "Fixing an issue where reference count increases were not applied in certain cases (#987)"

This reverts commit ed60c81.

* Revert "Splitting out general purpose helpers for branches and loops that yield values (#985)"

This reverts commit 34bd1fe.

* Revert "Ref count optimizations and some bug fixes (#982)"

This reverts commit 9689a96.

Co-authored-by: Bettina Heim <[email protected]>
@bettinaheim
Copy link
Contributor Author

Very briefly outlining the issue that this PR ran into and that I overlooked initially:
In order to ensure that a value stays alive when you return it, the ref counts for e.g. tuples and their items is 1 (so far so good), and the idea is that the calling function then releases them recursively. That is implemented this way with this PR and ok so far. The issue now is when we have a copy-and-update expression involving that. If that's the case, then we run into the same issue as described in #859; I had removed the adjustment introduced in 859, because I had thought it would no longer be necessary (or correct for that matter), which is incorrect.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[QIR] Array Concatenation is missing ref count increase

3 participants