Skip to content

[FBGEMM] Modularize FBGEMM CMake Target Definitions, pt 3b #4524

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

q10
Copy link
Contributor

@q10 q10 commented Jul 18, 2025

No description provided.

Copy link

netlify bot commented Jul 18, 2025

Deploy Preview for pytorch-fbgemm-docs ready!

Name Link
🔨 Latest commit 8f8617f
🔍 Latest deploy log https://app.netlify.com/projects/pytorch-fbgemm-docs/deploys/6883c2747b774800088e888e
😎 Deploy Preview https://deploy-preview-4524--pytorch-fbgemm-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@meta-cla meta-cla bot added the cla signed label Jul 18, 2025
@q10 q10 changed the title [FBGEMM][PR] [fbgemm] Modularize FBGEMM CMake Target Definitions, pt 3 [FBGEMM] Modularize FBGEMM CMake Target Definitions, pt 3 Jul 18, 2025
@q10 q10 force-pushed the bm/fbgemm-arm-build-3 branch 2 times, most recently from 2d2de86 to a6184c2 Compare July 19, 2025 04:22
@cyyever
Copy link
Contributor

cyyever commented Jul 19, 2025

In PT we use the Sanitizer::address and other similar targets for sanitizers, which are more accurate. Therefore should we check the existence of these targets and prefer them?

@q10 q10 force-pushed the bm/fbgemm-arm-build-3 branch 2 times, most recently from f54d31a to 752294e Compare July 21, 2025 22:52
-Wno-infinite-recursion
-Wno-sign-compare
-Wno-gnu-zero-variadic-macro-arguments
-Wno-unused-parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

We can enable -Wunused-parameter, the code is clean enough to support it.

if(CMAKE_COMPILER_IS_GNUCXX AND (CMAKE_CXX_COMPILER_VERSION VERSION_LESS 8.3.0))
# Workaround for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80947
list(APPEND lib_cc_flags
-Wno-attributes)
Copy link
Contributor

@cyyever cyyever Jul 22, 2025

Choose a reason for hiding this comment

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

Should we really support old g++? Is it for META builds? but how can they build C++20 code?

@cyyever
Copy link
Contributor

cyyever commented Jul 22, 2025

Some disabled-Wno-XXX warnings can actually be used.

@cyyever
Copy link
Contributor

cyyever commented Jul 22, 2025

One more concern: we use cpuinfo for runtime CPU architecture detection; meanwhile we also use conditional preprocessing inclusion for build time CPU architecture, are the actions conflicting?

@q10 q10 force-pushed the bm/fbgemm-arm-build-3 branch 3 times, most recently from 32d9fc3 to b83aa34 Compare July 23, 2025 22:56
@q10 q10 changed the title [FBGEMM] Modularize FBGEMM CMake Target Definitions, pt 3 [FBGEMM] Modularize FBGEMM CMake Target Definitions, pt 3b Jul 24, 2025
@q10 q10 force-pushed the bm/fbgemm-arm-build-3 branch from b83aa34 to 47fb27a Compare July 24, 2025 06:28
@q10 q10 force-pushed the bm/fbgemm-arm-build-3 branch from 47fb27a to d89366e Compare July 25, 2025 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants