Skip to content

Conversation

@mcr229
Copy link
Contributor

@mcr229 mcr229 commented Jun 24, 2025

Light refactoring to move config srcs into build_srcs.bzl

@mcr229
Copy link
Contributor Author

mcr229 commented Jun 24, 2025

@dsharletg

build_srcs.bzl Outdated
"src/xnnpack/vunary.h",
]

UKERNEL_CONFIG_SRCS = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think this should be MICROKERNEL_CONFIG_SRCS for consistency with e.g. MICROKERNEL_DEPS, MICROKERNEL_HDRS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea that makes sense.

Copy link
Collaborator

@dsharlet dsharlet left a comment

Choose a reason for hiding this comment

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

@gonnet FYI. Motivation here is, @mcr229 and team were using this variable in their build system. We deleted it because it was unused (in this repo). I think the best way to ensure it doesn't get deleted is to use it.

@mcr229 mcr229 force-pushed the refactor_config_srcs branch from ed52043 to f150b0c Compare June 25, 2025 18:37
@mcr229 mcr229 force-pushed the refactor_config_srcs branch from f150b0c to 6c9990a Compare July 1, 2025 01:50
@mcr229 mcr229 force-pushed the refactor_config_srcs branch from 6c9990a to 906ead5 Compare July 1, 2025 01:56
@mcr229
Copy link
Contributor Author

mcr229 commented Jul 1, 2025

Seems like the issue was that src/pack-lh.cc actually uses the unary_elementwise_config and reduce_config. So i kept those around and but those as deps for pack_lh. I removed all others and joined them into microkernel_configs.

@gonnet @dsharlet

hdrs = [
"//:src/xnnpack/config.h",
],
srcs = MICROKERNEL_CONFIG_SRCS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is reduce-config.c and unary-elementwise-config.c getting compiled twice now?

Can src/pack-lh.c just depend on all the configs, rather than treating the reduce and unary elementwise configs as "special"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That won't work because pack-lh-config, which is also in this target, depends on pack-lh, and that would cause a circular dependency.

@gonnet
Copy link
Collaborator

gonnet commented Jul 1, 2025

What is the purpose of removing the granular targets in src/configs/BUILD?

I'm asking because I only recently split these out because it's best practices to have as granular targets as possible (see e.g. here).

@dsharlet
Copy link
Collaborator

dsharlet commented Jul 1, 2025

I've sent #8664 which should eliminate the special case requirements of pack-lh, which would enable this PR to work as it was originally sent.

If sharing the BUILD becomes easier due to this change (a single BUILD target with a variable for the config srcs), then I don't think we should worry about bazel's guidelines @mcr229.

@gonnet
Copy link
Collaborator

gonnet commented Jul 1, 2025

@mcr229 I'm still confused about the intent of this change. If you need the list of config files, it's just the source files in XNNPACK/src/configs/.

The granular targets were part of an ongoing larger refactoring trying to put things in specific subdirectories to avoid having to have lists of sources (subgraph and operators were going to be next, but blocked on some dependency issues).

@mcr229
Copy link
Contributor Author

mcr229 commented Jul 1, 2025

@gonnet I didn't know that separating these build targets was an active effort. Honestly the level of separation of all MICROKERNEL_CONFIG srcs living in src/configs, all OPERATOR_SRCS living in src/operators, etc. makes sense and that level of organization could be leveraged by our build system to scrape the srcs from the appropriate directory. I'm ok with dropping this PR if that is the intent.

@gonnet
Copy link
Collaborator

gonnet commented Jul 1, 2025

@mcr229 That is fortunately almost already the current state, and I have an open Issue to clean things up. If it makes your build process simpler and/or easier to manage, I can prioritize wrapping up this work and get your feedback on the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants