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

[mypyc] Remove symbol table and value index table from Environment #9768

Merged
merged 11 commits into from
Dec 28, 2020

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Nov 29, 2020

Instead of maintaining symbol tables in Environment, they are maintained in
IRBuilder and discarded after they are no longer needed.

I also removed the value index table (indexes) from Environment. It was
mostly used for three purposes:

  1. Finding all values in a function
  2. Finding argument registers
  3. Sorting values in a predictable order

These use cases are now handled differently:

  1. I added functions, including all_values, that find values in IR by
    iterating over it and collecting them.
  2. FuncIR now stores argument registers explicitly.
  3. I added a helper function to produce an ordering for values
    (make_value_ordering).

There are three main reasons why I think that this PR makes sense. First, it
simplifies the IR and makes things easier to understand. Second, transforming
IR is easier and less error-prone, as the symbol tables and index dictionaries
no longer need to be kept in sync with the IR. Third, this makes it easy to get
rid of Environment altogether (in a follow-up PR).

A nice side effect is that we can now just create a Register instance directly
and start using it, instead of having to call alloc_temp().

Some tests had to be updated, since the registers are now numbered differently
in some cases. Arguably, it's now more logical and predictable. There won't be
gaps in the numbering.

Work on mypyc/mypyc#781.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Dec 6, 2020

Hopefully #9782 will help with the build failures.

@TH3CHARLie
Copy link
Collaborator

Thanks for fixing the build! I took a quick and it looks great! I'll do a full review tomorrow

@JukkaL
Copy link
Collaborator Author

JukkaL commented Dec 28, 2020

I'm going to merge this now since I have a bunch of other PRs I'm working on that depend on this. Code review after the fact would still be appreciated, and I will create a follow-up PR to address feedback.

@JukkaL JukkaL merged commit 7664031 into master Dec 28, 2020
@JukkaL JukkaL deleted the refactor-env-2 branch December 28, 2020 12:41
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.

None yet

2 participants