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

[cmake] Handful of small tweaks #567

Merged

Conversation

ChrisCummins
Copy link
Contributor

@ChrisCummins ChrisCummins commented Feb 8, 2022

This patch set:

  • Removes the "subprocess" library from the bazel and CMake build configs, as it is unused as of Replace subprocess::Popen() with boost::process #398.
  • Fixes a typo s/string/STRING/ that caused a harmless error to be printed.
  • Removes Polly from the CMake build config. It is not used by the LLVM environment. It is only included in the Bazel build only as a workaround for a weird bug in my Bazel LLVM config.
  • Strips the include/ prefix from all LLVM header includes, and the workaround in the CMake config.
  • Improves the ad-hoc code for parsing LLVM sources to extract the transform pass names, fixing a couple of bugs I discovered while building against LLVM 13.0.0.1.

The polly project is not used by CompilerGym.
Fixes the (harmless) typo "string" in place of "STRING", causing the
following warning:

    -- Install configuration: "Release"
    CMake Warning (dev) at build_tools/cmake/FindCsmith.cmake:52 (set):
      implicitly converting 'string' to 'STRING' type.
    Call Stack (most recent call first):
      external/external.cmake:397 (find_package)
      CMakeLists.txt:64 (include)
    This warning is for project developers.  Use -Wno-dev to suppress it.
@ChrisCummins ChrisCummins requested a review from hughleat February 8, 2022 20:08
@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 Feb 8, 2022
@ChrisCummins
Copy link
Contributor Author

cc @sogartar (I can't add you as a reviewer)

Copy link

@sogartar sogartar left a comment

Choose a reason for hiding this comment

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

I don't have any remarks. Since you mention that parse_initialize_pass is a "throw-away" code, I was not thorough with its review.

This makes the (dodgy AF) C++ parsing logic slightly more robust by
fixing a couple of bugs where concatenated string were not correctly
interpreted, and where comments were not parsed properly.
@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

Merging #567 (2c2e81e) into development (22ac844) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development     #567   +/-   ##
============================================
  Coverage        88.06%   88.06%           
============================================
  Files              114      114           
  Lines             6693     6693           
============================================
  Hits              5894     5894           
  Misses             799      799           

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 22ac844...2c2e81e. Read the comment docs.

@ChrisCummins ChrisCummins merged commit 8a3f963 into facebookresearch:development Feb 16, 2022
@ChrisCummins ChrisCummins deleted the feature/cmake-tweaks branch February 16, 2022 16:49
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.

4 participants