Add support for IR2Vec#615
Add support for IR2Vec#615ChrisCummins wants to merge 15 commits intofacebookresearch:developmentfrom
Conversation
ChrisCummins
left a comment
There was a problem hiding this comment.
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.dtypeenv.observation.spaces["Ir2VecFlowSensitive"].space.shapeenv.observation.spaces["Ir2VecFlowSensitive"].space.contains()env.observation.spaces["Ir2VecFlowSensitive"].deterministicenv.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
| } | ||
| case LlvmObservationSpace::IR2VEC_FA: { | ||
| ScalarRange featureSize; | ||
| featureSize.mutable_min()->set_value(0.0); |
There was a problem hiding this comment.
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.
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.
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.
It's a single 300 dimension vector
There was a problem hiding this comment.
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
If there is no lower bound, remove this line:
featureSize.mutable_min()->set_value(0);There was a problem hiding this comment.
OKay, it should be an
int64_listthen. 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
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.
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.