Skip to content

Audit Compiler build infrastructure (part 1)#1589

Merged
dime10 merged 14 commits intomainfrom
warnings-as-errors-3
Apr 3, 2025
Merged

Audit Compiler build infrastructure (part 1)#1589
dime10 merged 14 commits intomainfrom
warnings-as-errors-3

Conversation

@dime10
Copy link
Copy Markdown
Contributor

@dime10 dime10 commented Mar 28, 2025

The primary, high-level goal is to enable stricter safety checks in our build systems. In addition to stricter checks, ideally, all our build recipes should produce clean, warning-free outputs, as ignoring one set of warnings can easily lead to overlooking other warnings.

This PR takes a look at the compiler (mlir) and improves the following:

  • provide a warning-free (cmake+compiler) build across tested platforms
  • enable warnings as errors and -Wall by default

The above was tested on the runtime with the following configurations:

make targets: dialects
platforms: mac + apple clang 16, linux + clang 18, linux + gcc 13

The PR does not test or enable ASAN yes, it mainly deals with warnings.

@dime10 dime10 added compiler Pull requests that update the compiler housecleaning labels Mar 28, 2025
Copy link
Copy Markdown
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

Awesome.

@github-actions
Copy link
Copy Markdown
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@erick-xanadu erick-xanadu added the reviewer:require-wheels Pull Requests will need wheel building job successful before being merged label Mar 28, 2025
Base automatically changed from warnings-as-errors to main March 31, 2025 21:28
dime10 added 13 commits April 1, 2025 14:22
Resolves the following warning according to the suggested solution:

  CMake Warning (dev) at FetchContent.cmake:1373 (message):
    The DOWNLOAD_EXTRACT_TIMESTAMP option was not given and policy CMP0135 is
    not set.  The policy's OLD behavior will be used.  When using a URL
    download, the timestamps of extracted files should preferably be that of
    the time of extraction, otherwise code that depends on the extracted
    contents might not be rebuilt if the URL changes.  The OLD behavior
    preserves the timestamps from the archive instead, but this is usually not
    what you want.  Update your project to the NEW behavior or specify the
    DOWNLOAD_EXTRACT_TIMESTAMP option with a value of true to avoid this
    robustness issue.
  Call Stack (most recent call first):
    lib/Ion/Transforms/CMakeLists.txt:38 (FetchContent_Declare)
By default, stim will use the host's vectorization support when
compiling. For distributed packages, this could be a problem on other
machines.
Set a "reasonable" SIMD register width that Lightning also uses.
`*include_directories`, `FetchContent_Declare`, and `add_subdirectory`
accept the SYSTEM keyword in order to mark headers from those sources as
system headers. Typically, compilers will not emit warnings when
processing system headers.
Resolves the following warning:

  ld: warning: -undefined suppress is deprecated
Resolves the following warning:

  warning: control reaches end of non-void function [-Wreturn-type]
Resolves the issue where a member type was declared in an anonymous
namespace:

  error: ‘catalyst::ion::OQDDatabaseManager’ has a field ‘std::vector<{anonymous}::Beam> whose type has internal linkage [-Werror=subobject-linkage]
    29 | class OQDDatabaseManager {
.. which are being raised inside of LLVM.
@dime10 dime10 force-pushed the warnings-as-errors-3 branch from a8fbdf6 to 152a01d Compare April 1, 2025 21:29
@dime10 dime10 added the author:build-wheels Run the wheel building workflows on this Pull Request label Apr 1, 2025
@dime10 dime10 mentioned this pull request Apr 1, 2025
14 tasks
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.46%. Comparing base (e5ba25c) to head (a4985c1).
Report is 192 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1589      +/-   ##
==========================================
- Coverage   96.61%   96.46%   -0.16%     
==========================================
  Files          80       80              
  Lines        8578     8552      -26     
  Branches      819      819              
==========================================
- Hits         8288     8250      -38     
- Misses        234      246      +12     
  Partials       56       56              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

at catalyst/mlir/lib/Driver/CompilerDriver.cpp:505:
gcc-toolset-12/root/usr/include/c++/12/bits/char_traits.h:431:56:
  error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ accessing 9223372036854775810 or more bytes at offsets [2, 9223372036854775807] and 1 may overlap up to 9223372036854775813 bytes at offset -3 [-Werror=restrict]
  431 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
@dime10 dime10 force-pushed the warnings-as-errors-3 branch from d135b4d to a4985c1 Compare April 2, 2025 17:30
@dime10
Copy link
Copy Markdown
Contributor Author

dime10 commented Apr 2, 2025

@erick-xanadu The last four commits are new if you want to have a look at those again :) I removed the Python extension modules from Catalyst as discussed, and some additional warning suppressions.

@erick-xanadu
Copy link
Copy Markdown
Contributor

@dime10 can we wait until after the release?

@erick-xanadu
Copy link
Copy Markdown
Contributor

Ok! Good to go :)

@dime10 dime10 added author:build-wheels Run the wheel building workflows on this Pull Request and removed author:build-wheels Run the wheel building workflows on this Pull Request labels Apr 3, 2025
@dime10 dime10 merged commit 67ca97e into main Apr 3, 2025
113 of 118 checks passed
@dime10 dime10 deleted the warnings-as-errors-3 branch April 3, 2025 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author:build-wheels Run the wheel building workflows on this Pull Request compiler Pull requests that update the compiler housecleaning reviewer:require-wheels Pull Requests will need wheel building job successful before being merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants