New graph-coloring-based register allocator #816

mohammadfawaz · 2022-02-19T03:03:49Z

Closes #155 and #800

This is basically Chaitin's original graph-coloring based algorithm for register allocation. The new file that I that added, called register_allocator.rs, has all the steps in details. I tried to make the steps and the different pieces of the algorithm as clear as possible. I couldn't really find one source where the full algorithm in details is described. However, my comments should contain the union of what I found in the literature and how it applies to the data structures we have in CodeGen.

With this, the bytecode size typically goes down due to the coalescing phase. The table below shows the bytecode size saving percentages. I noticed that the larger the code, the bigger the savings.

Pipeline	Min	Max	Average
Without IR	0%	56%	17%
With IR	0%	58%	16%

Also, this patch fixes the functional issue described in #800. Loops introduce complexities to liveness analysis due to all the backward jumps, so I suspect something is going wrong with the existing allocator.

Now that I have all the building blocks, I can experiment with other algorithms such as the iterartive one described in https://c9x.me/compile/bib/irc.pdf. The steps are basically the same with minor tweeks and run in an iterative outer loop.

Update: E2E tests with the IR pipeline enabled seem to be okay except for three tests that are currently failing on master and the test that I'm adding here (due to #821).

mohammadfawaz · 2022-02-19T03:28:55Z

Some IR tests are failing because they check for exact the ASM output which is different now. I will update them. Converting to draft until I’m done.

mohammadfawaz · 2022-02-20T03:47:49Z

Ready for review.

@otrho - regarding the IR tests that I had to update: how did you figure out/check the assembly for these tests? Did you do it by inspection?

otrho · 2022-02-20T03:57:14Z

@otrho - regarding the IR tests that I had to update: how did you figure out/check the assembly for these tests? Did you do it by inspection?

Yeah, when you're adding them progressively it's fairly easy to eyeball them and confirm they're correct. But making a pervasive change to codegen like this means there's a fair bit of work confirming the new ASM is right.

At the same time you're trusting that the E2E regression tests are confirming that the changes haven't broken behaviour so I guess I'd be checking for stuff which looks obviously wrong in the IR tests, rather than making sure they're absolutely doing the right thing.

To run E2E with IR you can use the SWAY_TEST_USE_IR=1 environment variable. Note, at the moment there are 3 tests on master which are failing -- the recent generics and traits stuff you added, which I mentioned to you last week.

mohammadfawaz · 2022-02-20T04:26:07Z

@otrho - regarding the IR tests that I had to update: how did you figure out/check the assembly for these tests? Did you do it by inspection?

Yeah, when you're adding them progressively it's fairly easy to eyeball them and confirm they're correct. But making a pervasive change to codegen like this means there's a fair bit of work confirming the new ASM is right.

At the same time you're trusting that the E2E regression tests are confirming that the changes haven't broken behaviour so I guess I'd be checking for stuff which looks obviously wrong in the IR tests, rather than making sure they're absolutely doing the right thing.

To run E2E with IR you can use the SWAY_TEST_USE_IR=1 environment variable. Note, at the moment there are 3 tests on master which are failing -- the recent generics and traits stuff you added, which I mentioned to you last week.

I'll inspect the changes then - it would also be a good exercise to help understand the IR better :)
That being said, the E2E tests with the IR pipeline enabled seem to be okay except for the three tests that you mentiond and the test that I added in this PR (due to #821), so I think we're okay!

otrho

Looks good. Not sure why the new test would fail for IR though?

otrho · 2022-02-21T08:50:27Z

sway-core/src/asm_lang/virtual_ops.rs

@@ -223,21 +223,575 @@ impl VirtualOp {
        .collect()
    }



For all these giant functions I feel like there must be a more data driven compile time computational way to build these, with macros or something. But I don't know specifically the best way.

otrho · 2022-02-21T08:52:29Z

sway-core/tests/ir_to_asm/let_reassign_while_loop.asm

-move $r1 $r2
-ji   i8
-move $r0 $r1
+ji   i7
 ret  $r0


emilyaherbert

Ahh so cool, great job 😄

emilyaherbert

Actually one nit, would you be able to include the name of the algorithm (and possibly a link to the wikipedia page) in the docstring associated with the function? That way we won't have to dig for this issue if/when we need to expand on this code in the future 😄

mohammadfawaz · 2022-02-23T14:32:29Z

Actually one nit, would you be able to include the name of the algorithm (and possibly a link to the wikipedia page) in the docstring associated with the function? That way we won't have to dig for this issue if/when we need to expand on this code in the future 😄

Done!

mohammadfawaz · 2022-02-23T14:33:56Z

Looks good. Not sure why the new test would fail for IR though?

It's due to #821 which is an issue on master. I see you're already looking at it though, thanks! :)

emilyaherbert

Great job!

New graph-coloring-based register allocator

f299435

mohammadfawaz requested review from sezna, otrho and emilyaherbert February 19, 2022 03:04

mohammadfawaz self-assigned this Feb 19, 2022

mohammadfawaz added compiler General compiler. Should eventually become more specific as the issue is triaged enhancement New feature or request bug Something isn't working labels Feb 19, 2022

mohammadfawaz marked this pull request as draft February 19, 2022 04:22

Fixing up IR tests

64c0a1d

mohammadfawaz force-pushed the new_register_allocator branch from 7e04ba0 to 8466803 Compare February 20, 2022 03:23

some minor upadates to the comments and a few other things

634fc22

mohammadfawaz force-pushed the new_register_allocator branch from 8466803 to 634fc22 Compare February 20, 2022 03:27

mohammadfawaz marked this pull request as ready for review February 20, 2022 03:46

otrho previously approved these changes Feb 21, 2022

View reviewed changes

emilyaherbert approved these changes Feb 22, 2022

View reviewed changes

emilyaherbert self-requested a review February 22, 2022 17:15

emilyaherbert suggested changes Feb 22, 2022

View reviewed changes

Improving some comments

4de1320

mohammadfawaz dismissed otrho’s stale review via 4de1320 February 23, 2022 14:29

fmt

f3941f2

sezna approved these changes Feb 23, 2022

View reviewed changes

Merge branch 'master' into new_register_allocator

20266b5

mohammadfawaz requested a review from emilyaherbert February 23, 2022 14:49

emilyaherbert approved these changes Feb 23, 2022

View reviewed changes

mohammadfawaz merged commit 24bb7aa into FuelLabs:master Feb 23, 2022

mohammadfawaz deleted the new_register_allocator branch February 23, 2022 19:03

mohammadfawaz mentioned this pull request Feb 24, 2022

Graph-coloring register allocator #155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New graph-coloring-based register allocator #816

New graph-coloring-based register allocator #816

mohammadfawaz commented Feb 19, 2022 •

edited

Loading

mohammadfawaz commented Feb 19, 2022 •

edited

Loading

mohammadfawaz commented Feb 20, 2022

otrho commented Feb 20, 2022

mohammadfawaz commented Feb 20, 2022 •

edited

Loading

otrho left a comment •

edited

Loading

otrho Feb 21, 2022

otrho Feb 21, 2022

emilyaherbert left a comment

emilyaherbert left a comment •

edited

Loading

mohammadfawaz commented Feb 23, 2022

mohammadfawaz commented Feb 23, 2022

emilyaherbert left a comment

New graph-coloring-based register allocator #816

New graph-coloring-based register allocator #816

Conversation

mohammadfawaz commented Feb 19, 2022 • edited Loading

mohammadfawaz commented Feb 19, 2022 • edited Loading

mohammadfawaz commented Feb 20, 2022

otrho commented Feb 20, 2022

mohammadfawaz commented Feb 20, 2022 • edited Loading

otrho left a comment • edited Loading

Choose a reason for hiding this comment

otrho Feb 21, 2022

Choose a reason for hiding this comment

otrho Feb 21, 2022

Choose a reason for hiding this comment

emilyaherbert left a comment

Choose a reason for hiding this comment

emilyaherbert left a comment • edited Loading

Choose a reason for hiding this comment

mohammadfawaz commented Feb 23, 2022

mohammadfawaz commented Feb 23, 2022

emilyaherbert left a comment

Choose a reason for hiding this comment

mohammadfawaz commented Feb 19, 2022 •

edited

Loading

mohammadfawaz commented Feb 19, 2022 •

edited

Loading

mohammadfawaz commented Feb 20, 2022 •

edited

Loading

otrho left a comment •

edited

Loading

emilyaherbert left a comment •

edited

Loading