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

[SSA] Fix up to_ssa.py and add test cases #119

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

terrelln
Copy link
Contributor

@terrelln terrelln commented Jan 12, 2021

This PR contains 4 commits detailed below. In summary the goal is to remove all uses of undefined variables. It fixes all the problems of Issue #108. It is done in 3 steps:

  1. Add support for null pointers to brili and brilirs, created with alloc 0.
  2. Fix a bug in rename() that caused references to undefined variables (not __undefined) to be emitted.
  3. Define __undefined.${type} for each type at the top of the entry block, and use __undefined.${type} instead of __undefined.

After these steps phi nodes no longer have undefined arguments, and no longer produce undefined results. The ability to execute phi nodes with undefined or missing arguments could be removed from brili, or hidden behind a flag (but I haven't done that).

I added one test case to_ssa/ called loop-branch.bril which has an interesting feature where two phis form a dependency loop: v.0 depends on v.1 and visa-versa.

Then I add test cases for from_ssa.py that executes the round-tripped code for every test in to_ssa/. I also tested on all the programs in the benchmarks directory, and it works on everything in that directory. I haven't added them to the from_ssa tests to avoid adding overly complex tests.

This newly generated code inserts potentially unnecessary __undefined.${type} definitions. A DCE pass can trivially remove the unnecessary ones.


[brili][memory] Support null pointers

  • Null pointers are created by alloc 0.
  • All null pointers share the same key Key(0, 0).
  • Freeing a null pointer is a no-op.
  • Null pointers do not need to be freed.
  • Reading/writing a null pointer is an error.
  • Pointer addition is allowed on null pointers.

[to_ssa] Fix bug in rename()

The use of defaultdict() and stack.update(old_stack) meant that any
stacks that were empty at the top of _rename() weren't cleared at the
bottom. This caused use of undefined variables inserted into phi
instead of __undefined.


[to_ssa] Define undefined variables

Break undefined variables up by type, to meet the bril spec that each
variable has only one type. Then, define the undefined variable for each
type in the entry block. This ensures that no phi node uses undefined
variables, or produces undefined variables, in any case.

Add an interesting test case loop-branch.bril which produces phi nodes
that form a dependency loop.


[from_ssa] Add test cases

Add brili test cases, since I want to test that no undefined varaibles
are used during execution after from_ssa.

@terrelln terrelln changed the title [SSA] Fix up to_ssa.py and add test cases [SSA] Fix up to_ssa.py and add test cases Jan 12, 2021
@terrelln
Copy link
Contributor Author

I think the allowance of undefined variables in phi nodes may have been a mistake. The wrinkle that phi nodes are allowed undefined values, but no other instruction is, really complicates analysis of the bril language.

I think that there are two logical choices, and anything in the middle will cause headaches:

  1. Strictly allow no undefined variables.
  2. Allow undefined variables in all ops. Any operation with undefined variables is undefined.

@cgyurgyik
Copy link
Contributor

Cool! This is probably much cleaner than my approach. The end result is the same, having to define an __undefine. Couldn't this be removed with a pass before emitting code in SSA form (as you mentioned earlier)?


def insert_undefined_vars(blocks, types):
entry = list(blocks.keys())[0]
types = sorted(set(types.values()))
Copy link
Contributor

@cgyurgyik cgyurgyik Jan 12, 2021

Choose a reason for hiding this comment

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

I believe this will crash when using memory. For example,

site: ptr<int> = alloc n;

bril2json < benchmarks/eight-queens.bril | python3 examples/to_ssa.py
The pointer type is stored in a dict, and thus unhashable.

Reference: https://capra.cs.cornell.edu/bril/lang/memory.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, is this something that to_ssa.py should support? If so I can add it. There currently isn't a null value for pointers, but I suppose alloc 0; would work, and just never free.

@terrelln terrelln force-pushed the master branch 2 times, most recently from 65fc822 to c89a009 Compare January 16, 2021 18:45
@terrelln
Copy link
Contributor Author

I've added support for the memory extension to to_ssa.py. In order to do that, I needed to support null pointers in brili and brilirs. Null pointers are created with alloc 0, and do not need to be freed. See the first commit message for details.

I've decided to use alloc 0, so as not to introduce another instruction, or make const memory aware.

* Null pointers are created by `alloc 0`.
* All null pointers share the same key `Key(0, 0)`.
* Freeing a null pointer is a no-op.
* Null pointers do not need to be freed.
* Reading/writing a null pointer is an error.
* Pointer addition is allowed on null pointers.
The use of `defaultdict()` and `stack.update(old_stack)` meant that any
stacks that were empty at the top of `_rename()` weren't cleared at the
bottom. This caused use of undefined variables inserted into `phi`
instead of `__undefined`.

The fix is to copy the stack at the top of _rename(). This is just as
efficient as before (not very efficient).
Break undefined variables up by type, to meet the bril spec that each
variable has only one type. Then, define the undefined variable for each
type in the entry block. This ensures that no phi node uses undefined
variables, or produces undefined variables, in any case.

Add an interesting test case `loop-branch.bril` which produces phi nodes
that form a dependency loop.
Add `brili` test cases, since I want to test that no undefined varaibles
are used during execution after `from_ssa`.
sampsyo added a commit that referenced this pull request Aug 23, 2021
sampsyo added a commit that referenced this pull request Aug 23, 2021
sampsyo added a commit that referenced this pull request Aug 23, 2021
This is similar to the functional stack passing fix in #119 but has a
smaller fix (just clobber the stacks altogether before restoring the old
values; previously we just overwrote the new variables).
sampsyo added a commit that referenced this pull request Aug 23, 2021
More tests and a fix for SSA conversion (from #119)
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