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

Improve llvm-config.h to record which target is configured or not #71164

Conversation

joker-eph
Copy link
Collaborator

These macro can help guarding some tests and other section of code
which rely on detecting if a particular target is available. This
is common the MLIR codegeneration for GPU targets for example.

Created using spr 1.3.4
@joker-eph joker-eph merged commit 3fe69ba into main Nov 3, 2023
2 checks passed
@joker-eph joker-eph deleted the users/joker-eph/improve-llvm-configh-to-record-which-target-is-configured-or-not branch November 3, 2023 18:01
@nico
Copy link
Contributor

nico commented Nov 3, 2023

Before this change,. toggling on or off a target required building fairly few files. Now it's a full rebuild. Any chance we could use a different approach here?

@joker-eph
Copy link
Collaborator Author

Is this a common thing to switch back and forth? Also isn’t ccache good enough cases where one would want to back and forth?

@joker-eph
Copy link
Collaborator Author

Actually: the header change will invalidate ccache

@joker-eph
Copy link
Collaborator Author

Thinking a bit more about this: seems to me that the underlying problem with respect to caching and rebuilding is that the llvm-config.h file is monolithic and every flag adds to the burden.
What about splitting it? We can have a subfolder configs/xxx.h for every flag (or grouping of flags) and the top-level llvm-config.h including all of them.
Then it's a matter of migrating current includes of llvm-config.h to include the specific configs/xxx.h, this should provide the finer grain rebuild we could expect?

@fabianmcg
Copy link
Contributor

That would solve the issue, however, that's a big refactor. How about, just creating llvm-targets.h and moving these flags there?

@joker-eph
Copy link
Collaborator Author

I don't see anything more special about these than any other flags right now though.

@fabianmcg
Copy link
Contributor

You're right, I was just proposing avoiding a bigger refactor as splitting it means going through each include of llvm-config.h and only including the necessary files. I think Targets.h, Version.h, TargetDefault.h, BuildOptions.h would do it.

@nico
Copy link
Contributor

nico commented Nov 4, 2023

Splitting more would be nice, but doing the smaller thing might be easier.

@nico
Copy link
Contributor

nico commented Nov 4, 2023

Is this a common thing to switch back and forth?

I do it several times a week at least. I usually only build with host arch, and if I need to enable an additional arch to debug a per-arch thing I turn on that arch. That used to be very quick, and now it isn't. (As you say, it's a full rebuild without any cache hits.)

Maybe it could be a generated header in mlir for gpu targets only? Then it wouldn't affect me at all. Putting it in llvm-config.h just because the file already exists seems a bit weird – half of llvm-config.h are these defines now.

@joker-eph
Copy link
Collaborator Author

joker-eph commented Nov 5, 2023

I need to enable an additional arch to debug a per-arch thing I turn on that arch

I do this as well, but it hardly a frequent thing that I break another target. But then I don't work on SelectionDAG either (but if I was, I would keep all the targets enabled all the time: better ccache and better incremental rebuild anyway)

Maybe it could be a generated header in mlir for gpu targets only?

Does not make sense to me: this isn't an MLIR settings...

Putting it in llvm-config.h just because the file already exists seems a bit weird

Seems to me like this is the unfortunate design of llvm-config.h...

Please take a look at #71273 !

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.

4 participants