-
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
Implement LLVM-based Lexer for IR #742
Conversation
Not sure why 7 commits show up in the PR instead of just 2, probably because I branched from |
Codecov Report
@@ Coverage Diff @@
## development #742 +/- ##
===============================================
- Coverage 88.75% 88.73% -0.03%
===============================================
Files 129 130 +1
Lines 7855 7864 +9
===============================================
+ Hits 6972 6978 +6
- Misses 883 886 +3
|
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.
Looking good, thanks @fivosts! I've left some very nitpicky comments inline. Few small things:
- rebase on
development
. - you should run the pre-commit hooks here which will sadly obliterate all your nice vertical aligned code 🙂
- I think we need a copy of the LLVM License file at
compiler_gym/third_party/LexedIR/LICENSE.txt
. You can copy the one fromcompiler_gym/third_party/llvm/LICENSE.txt
. - I don't see any changes to the tests, so
development/tests/llvm/observation_spaces_test.py
should break. Could you add a test to cover the new space? Take a look here for an example
Cheers,
Chris
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.
Thanks for addressing my nitpicks. Here's a couple more 🙂
Something looks fishy about the git history here (130 commits!), may need rebasing/cherry-picking
Cheers,
Chris
@@ -0,0 +1 @@ | |||
/private/home/foivos/CompilerGym/cmake_build/compiler_gym/third_party/autophase/compute_autophase |
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.
Looks like this was checked in by accident
@@ -0,0 +1 @@ | |||
/private/home/foivos/CompilerGym/cmake_build/compiler_gym/third_party/programl/compute_programl |
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.
Same here
Note the "Pre-commit" job is failing with a few BUILD file errors. You could run buildifier locally or just copy the patch from the CI output |
Expose 'Lexedir' and 'Lexedirtuple' observations for LLVM-IR.
Co-authored-by: Chris Cummins <chrisc.101@gmail.com>
ab3a7c9
to
820abdc
Compare
Rebasing to dev probably did this. Did fresh rebase, removed broken symlinks, ran buildifier and cherry picked my commits, should be fine now. Let me know if there's anything else I have missed |
LGTM. The CI test failures are known problems and not caused by this change. Merging, thanks @fivosts! Cheers, |
Adds 2 observations: