Add initial cgra compiler #704

j-c-w · 2022-06-10T16:36:48Z

Hi,

This adds a CGRA scheduling environment.

There are two environments that are added: the cgra-v0 environment, which allows for direct placement (as in https://arxiv.org/abs/2205.13675) and the cgra-relative-v0 environment, which allows for relative placement (as in https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8509169).

Within examples, a genetic algorithm approach is implemented, which works for the relative environemnt.

The CGRA environment is mostly complete, but currently lacks a way of specifying a CGRA --- this is currently done within compiler_gym. Due to effects on the sizes of observation spaces, it will likely stay this way, but the input format for the CGRAs is the next step to be improved.

Jackson

ChrisCummins

Hey Jackson, thanks for the pull request!

I had a read through and left a comments inline but there's a lot of code here so it may take a couple of passes :)

Overall, this looks like a great start. Most of my comments are just small nitpicks. The two main suggestions I have to start with are:

We'll need some tests of the new environment before we can merge. You can take a look in the tests/llvm and tests/gcc directories for some examples to get you started. The goal is to cover all of the branches in your code, though to start with some simple integration tests are a good start. They help catch regressions (so that I don't accidentally break your code by changing something else), as well as providing usage pointers.
Each environments needs a documentation page to provide context on what it is, the problem that it exposes, the various observation/reward/datasets available, etc. See here for an example. To create one of these pages, take a look in docs/source/envs and copy and hack on one of the existing docs.

There also looks like there are code style inconsistencies. Some of these can be addressed automatically by running our pre-commit hooks. See here to get started.

Looking forward to landing the CGRA env!

Cheers,
Chris

compiler_gym/datasets/datasets.py

compiler_gym/envs/cgra/.gitignore

compiler_gym/envs/cgra/BUILD

ChrisCummins · 2022-06-10T22:49:07Z

compiler_gym/envs/cgra/CMakeLists.txt

@@ -0,0 +1,25 @@
+# Copyright (c) Facebook, Inc. and its affiliates.


I think this file needs updating to match the cgra BUILD file

compiler_gym/envs/cgra/DFG.py

ChrisCummins · 2022-06-10T22:58:42Z

compiler_gym/envs/cgra/service/cgra_service.py

+    # When this is set, the scheduler will take schedules
+    # that don't account for delays appropriately, and try
+    # to stretch them out to account for delays correctly.
+    # When this is false, the compiler will just reject
+    # such invalid schedules.
+    # (when set, something like x + (y * z), scheduled
+    # as +: PE0 on cycle 0, *: PE1 on cycle 0 is valid).
+    "IntroduceRequiredDelays": False


If this would be useful to expose to the user, we could consider doing so using the session parameter API (nothing actionable for this PR, but maybe add a TODO note

ChrisCummins · 2022-06-10T22:58:54Z

compiler_gym/envs/cgra/service/cgra_service.py

+    # TODO -- properly load CGRA
+    return CGRA(5, 5)


ChrisCummins · 2022-06-10T22:59:12Z

compiler_gym/envs/cgra/service/cgra_service.py

+        else:
+            return hop in self.schedule[time]
+
+# A class representing a NOC.


What does NOC stand for?

ChrisCummins · 2022-06-10T23:00:56Z

examples/cgra/.gitignore

@@ -0,0 +1,2 @@
+rewards.png


Is this needed?

ChrisCummins · 2022-06-10T23:02:29Z

setup.py

@@ -146,6 +149,8 @@ def wheel_filename(**kwargs):
    "package_data": {
        "compiler_gym": [
            "envs/gcc/service/compiler_gym-gcc-service",
+            # "envs/cgra/service/compiler_gym-cgra-service",


Suggested change

# "envs/cgra/service/compiler_gym-cgra-service",

codecov-commenter · 2022-06-10T23:35:01Z

Codecov Report

Merging #704 (bcc5663) into development (63dbfac) will decrease coverage by 27.57%.
The diff coverage is 24.50%.

@@               Coverage Diff                @@
##           development     #704       +/-   ##
================================================
- Coverage        88.36%   60.78%   -27.58%     
================================================
  Files              125      143       +18     
  Lines             7565     9000     +1435     
================================================
- Hits              6685     5471     -1214     
- Misses             880     3529     +2649

Impacted Files	Coverage Δ
...ym/envs/cgra/service/relative_placement_service.py	`0.00% <0.00%> (ø)`
compiler_gym/envs/cgra/DFG.py	`16.32% <16.32%> (ø)`
compiler_gym/envs/cgra/service/cgra_service.py	`17.52% <17.52%> (ø)`
compiler_gym/envs/cgra/cgra_rewards.py	`31.03% <31.03%> (ø)`
compiler_gym/envs/cgra/architectures/CGRA.py	`34.52% <34.52%> (ø)`
compiler_gym/envs/cgra/Operations.py	`50.00% <50.00%> (ø)`
...ompiler_gym/envs/cgra/service/relative_cgra_env.py	`54.54% <54.54%> (ø)`
compiler_gym/envs/cgra/datasets/__init__.py	`62.50% <62.50%> (ø)`
compiler_gym/envs/cgra/datasets/dfg_bench.py	`73.01% <73.01%> (ø)`
compiler_gym/envs/cgra/service/cgra_env.py	`76.00% <76.00%> (ø)`
... and 81 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63dbfac...bcc5663. Read the comment docs.

j-c-w · 2022-06-20T21:30:54Z

I've updated this pull request with most of the issues raised addressed. Test are not yet added, I will add those shortly.

ChrisCummins

Thanks for addressing the comments @j-c-w! I left a few more, but at this point its mostly nitpicks.

The tests & documentation are merge blocking, but this is shaping up well.

Cheers,
Chris

ChrisCummins · 2022-06-20T22:48:44Z

compiler_gym/envs/cgra/DFG.py

@@ -24,41 +29,26 @@ def __str__(self):
        return "Node with name " + self.name + " and op " + str(self.operation)

 class DFG(object):
-    def __init__(self, working_directory: Optional[Path] = None, benchmark: Optional[Benchmark] = None, from_json: Optional[Path] = None, from_text: Optional[str] = None):
+    def __init__(self, working_directory: Optional[Path] = None, from_json: Optional[Path] = None, from_text: Optional[str] = None):
        # Copied from here: https://github.com/facebookresearch/CompilerGym/blob/development/examples/loop_optimizations_service/service_py/loops_opt_service.py
        # self.inst2vec = _INST2VEC_ENCODER

        if from_json is not None:
            self.load_dfg_from_json(from_json)
        elif from_text is not None:
            self.load_dfg_from_text(from_text)


Does this need a final else branch to raise an error if neither is set?

ChrisCummins · 2022-06-20T22:49:48Z

compiler_gym/envs/cgra/DFG.py

+                    # cross-loop dependencies.
+                    print("Cyclical DFG --- Impossible to do a true BFS")
+                    print("DFG is ", str(self))
+                    assert False


Nitpick: don't assert False, raise an exception. Or, if that's not possible, sys.exit(1).

ChrisCummins · 2022-06-20T22:52:02Z

compiler_gym/envs/cgra/architectures/CGRA.py

+"""
+This is the most abstract representation of a CGRA ----
+a list of nodes, with an interconnect archtiecture.
+
+This can be inherited from to make things easier.
+"""
+class CGRA(object):


Docstrings need to be placed directly after the class/function/method declaration:

class CGRA: """blah blah""" ...

Don't put the docstring before, and don't use # comments instead of """ docstrings. This applies several places else in the PR.

Nitpick: don't subclass object, it's not necessary (again, applies elsewhere in the PR)

ChrisCummins · 2022-06-20T22:55:44Z

compiler_gym/envs/cgra/compile_settings.py

+# This source code is licensed under the MIT license found in the
+# LICENSE file in the root directory of this source tree.
+
+CGRACompileSettings = {


I would suggesting using pydantic to make this a class. It has a number of advantages: 1) docstrings can be rendered for documentation, (2) type validation and type safety, (3) automatic serialization/deserialization, (4) prevents typos

Here's an example of it in use in CompilerGym:

https://github.com/facebookresearch/CompilerGym/blob/development/compiler_gym/compiler_env_state.py#L19-L40

BTW, love the documentation of the settings, good job 🙂

ChrisCummins · 2022-06-20T22:57:41Z

examples/cgra/ga_scripts/plot.sh

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+python plot_CDFs.py ga_output GA ../relative_placement_output/rp_data RLMap


Does this need to be a script? 🙂 Or could it just be in a README.md file for this dir?

facebook-github-bot · 2022-12-10T08:16:04Z

Hi @j-c-w!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

Add initial cgra compiler

96b6f34

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 10, 2022

ChrisCummins reviewed Jun 10, 2022

View reviewed changes

Address most issues raised in merge request

bcc5663

ChrisCummins reviewed Jun 20, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial cgra compiler #704

Add initial cgra compiler #704

j-c-w commented Jun 10, 2022

ChrisCummins left a comment

ChrisCummins Jun 10, 2022

ChrisCummins Jun 10, 2022

ChrisCummins Jun 10, 2022

ChrisCummins Jun 10, 2022

ChrisCummins Jun 10, 2022

ChrisCummins Jun 10, 2022

codecov-commenter commented Jun 10, 2022 •

edited

Loading

j-c-w commented Jun 20, 2022

ChrisCummins left a comment

ChrisCummins Jun 20, 2022

ChrisCummins Jun 20, 2022

ChrisCummins Jun 20, 2022

ChrisCummins Jun 20, 2022

ChrisCummins Jun 20, 2022

ChrisCummins Jun 20, 2022

ChrisCummins Jun 20, 2022

facebook-github-bot commented Dec 10, 2022

		@@ -0,0 +1,25 @@
		# Copyright (c) Facebook, Inc. and its affiliates.

		@@ -0,0 +1,3 @@
		#!/bin/bash

		python plot_CDFs.py ga_output GA ../relative_placement_output/rp_data RLMap

Add initial cgra compiler #704

Are you sure you want to change the base?

Add initial cgra compiler #704

Conversation

j-c-w commented Jun 10, 2022

ChrisCummins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 10, 2022 • edited Loading

Codecov Report

j-c-w commented Jun 20, 2022

ChrisCummins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 10, 2022

Process

codecov-commenter commented Jun 10, 2022 •

edited

Loading