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

Add support for IR2Vec #615

Closed

Conversation

ChrisCummins
Copy link
Contributor

Moving @anilavakundu's PR (ChrisCummins#3) to the main repo. This supersedes #560.

Adds IR2Vec (https://github.com/IITH-Compilers/IR2Vec) an observation space for compiler gym.

Fixes #449.

@facebook-github-bot 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 Mar 8, 2022
@ChrisCummins ChrisCummins changed the title Feature/ir2vec Add support for IR2Vec Mar 8, 2022
Copy link
Contributor Author

@ChrisCummins ChrisCummins left a comment

Choose a reason for hiding this comment

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

Hi @anilavakundu, thanks for pushing on this! I've left some comments inline.

The one thing that is missing is this code needs tests. Take a look at the tests/llvm/observation_spaces_test.py file. Each observation space needs a test function. At a minimum, you will want to test these properties for each of the four new observation spaces:

  • env.observation.spaces["Ir2VecFlowSensitive"].space.dtype
  • env.observation.spaces["Ir2VecFlowSensitive"].space.shape
  • env.observation.spaces["Ir2VecFlowSensitive"].space.contains()
  • env.observation.spaces["Ir2VecFlowSensitive"].deterministic
  • env.observation.spaces["Ir2VecFlowSensitive"].platform_dependent

You can use the existing tests (like test_inst2vec_observation_space) as a starting point. To run the tests locally, see INSTALL.md.

Once you've done that, let me know. There's still a couple more things that need doing before merging (adding CMake support, rebasing on top of latest development branch) but I'm happy to do those for you.

Thanks again!

Cheers,
Chris

compiler_gym/envs/llvm/service/ObservationSpaces.h Outdated Show resolved Hide resolved
compiler_gym/envs/llvm/service/ObservationSpaces.h Outdated Show resolved Hide resolved
compiler_gym/envs/llvm/service/ObservationSpaces.cc Outdated Show resolved Hide resolved
@@ -90,6 +94,70 @@ std::vector<ObservationSpace> getLlvmObservationSpaceList() {
defaultValue.begin(), defaultValue.end()};
break;
}
case LlvmObservationSpace::IR2VEC_FA: {
ScalarRange featureSize;
featureSize.mutable_min()->set_value(0.0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this. This code says that values are in the range [0,∞], but when I run your code I see plenty of negative values:

>>> env.observation["Ir2vecFa"]
array([ 16.67847493,  -8.76068458, -49.90505585,  12.22742195,
        15.365804  , -10.31495722, -42.74864244, -10.26059285,
       -44.02249729,  23.81014121,  29.06372466,  24.08734658,
       -14.95525244,  19.8942756 , -29.91043964,  10.30582115,
       -24.02354845,   0.10980253,  -1.66427926,  14.17916835,
       -34.78192827,  37.14407874,  -8.33318256,  -3.45480279,
       -16.80741089, -32.45384884,  45.50566991,  37.82753753,
       -49.07060102,  -8.93597257, -52.5364784 ,   1.33546551,
       -12.41253508,  29.89899298,  10.97634208,  10.21049925,
        31.45356546,  16.61958681,  13.0980088 ,  -8.284721  ,

What are the bounds for embedding values?

Choose a reason for hiding this comment

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

It seems that ScalarRange wasn't a good fit for the shape of the embeddings as the range is not really bounded. I switched this to be of a Sequence type and fixed the length of the sequence type to be of 300 for both max & min. Can you please check the new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the shape of the two non-function-level spaces? Is it a single 300 dimension vector? Or a list of 300 dimension vectors?

Choose a reason for hiding this comment

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

It's a single 300 dimension vector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OKay, it should be an int64_list then. You can copy the Autophase space and adjust the dimensionality and limits:

ScalarRange featureSize;
featureSize.mutable_min()->set_value(0);
std::vector<ScalarRange> featureSizes;
featureSizes.reserve(kAutophaseFeatureDim);
for (size_t i = 0; i < kAutophaseFeatureDim; ++i) {
featureSizes.push_back(featureSize);
}
*space.mutable_int64_range_list()->mutable_range() = {featureSizes.begin(),
featureSizes.end()};
space.set_deterministic(true);
space.set_platform_dependent(false);
std::vector<int64_t> defaultValue(kAutophaseFeatureDim, 0);
*space.mutable_default_value()->mutable_int64_list()->mutable_value() = {
defaultValue.begin(), defaultValue.end()};

If there is no lower bound, remove this line:

        featureSize.mutable_min()->set_value(0);

Choose a reason for hiding this comment

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

OKay, it should be an int64_list then. You can copy the Autophase space and adjust the dimensionality and limits:

ScalarRange featureSize;
featureSize.mutable_min()->set_value(0);
std::vector<ScalarRange> featureSizes;
featureSizes.reserve(kAutophaseFeatureDim);
for (size_t i = 0; i < kAutophaseFeatureDim; ++i) {
featureSizes.push_back(featureSize);
}
*space.mutable_int64_range_list()->mutable_range() = {featureSizes.begin(),
featureSizes.end()};
space.set_deterministic(true);
space.set_platform_dependent(false);
std::vector<int64_t> defaultValue(kAutophaseFeatureDim, 0);
*space.mutable_default_value()->mutable_int64_list()->mutable_value() = {
defaultValue.begin(), defaultValue.end()};

If there is no lower bound, remove this line:

        featureSize.mutable_min()->set_value(0);

Did you mean a double_list ? The values for the embeddings are floating-point numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, of course, sorry :)

@ChrisCummins
Copy link
Contributor Author

Bouncing this back over to #560 now that I've updated onto the new proto schema!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add IR2Vec observation space
3 participants