-
Notifications
You must be signed in to change notification settings - Fork 129
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
Add support for IR2Vec #615
Conversation
There was a problem hiding this 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
@@ -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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
CompilerGym/compiler_gym/envs/llvm/service/ObservationSpaces.cc
Lines 81 to 94 in 1553e1f
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);
There was a problem hiding this comment.
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:CompilerGym/compiler_gym/envs/llvm/service/ObservationSpaces.cc
Lines 81 to 94 in 1553e1f
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
There was a problem hiding this comment.
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 :)
Bouncing this back over to #560 now that I've updated onto the new proto schema! |
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.