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

Disable incorrect SSU enforcement in Brili reference interpreter #412

Merged
merged 1 commit into from
Mar 12, 2025

Conversation

InnovativeInventor
Copy link
Contributor

@InnovativeInventor InnovativeInventor commented Mar 11, 2025

This line is incorrect. In particular, consider the following program as a counterexample:

@main() {
  one: int = const 1;
  set shadow one;
  main.cond: bool = const true;
  set cond main.cond;
.test:
  cond: bool = get;
  shadow: int = get;
  br cond .false .end;
.false:
  false.cond: bool = const false;
  set cond false.cond;
  jmp .test;
.end:
  print shadow;
}

I claim the following about the above program.

  1. It is in SSA.
  2. It follows the explicitly documented static and dynamic restrictions
    on set/get.
  3. It always terminates with the output 1.

However, when I run brili, I obtain the following output:

-> bril2json < repro.bril | brili
error: get without corresponding set for shadow

This commit disables the overly conservative enforcement mechanism. I suspect that SSU is best enforced via a static check rather than a dynamic runtime check.

@InnovativeInventor InnovativeInventor force-pushed the ssa-ssu-bug-fix branch 3 times, most recently from 933c60e to 195eb7b Compare March 11, 2025 04:37
@InnovativeInventor
Copy link
Contributor Author

@sampsyo CI seems to be failing on this PR and I'm not quite sure why. Perhaps I'm missing something, but the following error does not seem to be my fault:
(https://github.com/sampsyo/bril/actions/runs/13780468027/job/38537667879#step:17:2)

...
Error: Cache folder path is retrieved for pip but doesn't exist on disk: /home/runner/.cache/pip
...

@ethanuppal
Copy link
Contributor

I had the same issue on my PR - I think the general strategy on an open source project is to only ping the maintainer after a week of no reply

This line is incorrect. In particular, consider the following program as
a counterexample:
```
@main() {
  one: int = const 1;
  set shadow one;
  main.cond: bool = const true;
  set cond main.cond;
.test:
  cond: bool = get;
  shadow: int = get;
  br cond .false .end;
.false:
  false.cond: bool = const false;
  set cond false.cond;
  jmp .test;
.end:
  print shadow;
}
```
I claim the following about the above program.
1) It is in SSA.
2) It follows the explicitly documented static and dynamic restrictions
   on `set`/`get`.
3) It always terminates with the output `1`.

However, when I run `brili`, I obtain the following output:
```
-> bril2json < repro.bril | brili
error: get without corresponding set for shadow
```

I suspect that SSU is best enforced via a static check rather than a
dynamic runtime check.

This commit disables the overly conservative enforcement mechanism (and
removes the associated test). It also adds a regression test with the
above counterexample program.
@sampsyo
Copy link
Owner

sampsyo commented Mar 12, 2025

Thank you!! I fixed the CI trouble and rebased this branch. You're right that this check was just wrong and me being overzealous; let's remove it.

@sampsyo sampsyo merged commit 6a6911a into sampsyo:main Mar 12, 2025
3 checks 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