Rerun against tests. Previous failure was most likely unrelated to this change,
but let's make sure.

Rebase

Fat fingers.

Yayy :D

Hmm I tried to fiddle around with this but I'm out of my depth here. Pulling in @MaskRay @compnerd @phosek in the hopes that one of you is able to provide some insight or pull in someone else who is able to help.

There is no always-link in cmake as far as I know, so we should not need any in Bazel either in the LLVM project hopefully.

In D145258#4168143, @dblaikie wrote:

I would've thought it was necessary to alwayslink them because maybe they use some kind of auto-registration system into clang-tidy, so without alwayslink your clang-tidy binary wouldn't link any of them in?

I really like this so far. I think we need to make sure though that we don't confuse standard and non-standard modules though. If we want to test both, we should clearly label them, though it's probably a good idea to only use the intended-to-be-standard-conform variant for this prototype patch.

My bad, thanks!

I think unless conflicts arise creating an issue similar to this https://github.com/llvm/llvm-project/issues/60600 with the cherry-pick line set to this commit should be enough. (See also https://llvm.org/docs/GitHub.html).

I cannot say there was much choice. The only real choice was to postpone the split and magnify the problem in the future. As for the ifdefs, this might be possible in the device-libs but I do not see how to do it the Builtins.def.

Well, I can already feel the pain that distro maintainers having to build the next ROCm releases 😅

It shall be complimented by the device-lib change in the corresponding release, so it is not that simple.

Would it be possible to backport this to Clang 16?

Ok so I went over https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations#testing-for-the-presence-of-a-header-__has_include (thanks, @aaron.ballman 😊) and through the commit history for the amdgpu-arch tool.

Cool, thanks a lot for your efforts!

Looking at the include order in BUILD.bazel I think you need to run buildifier over this once, but other than that I'm very happy with this state of the diff.

Thanks for addressing. Also, really appreciate the tests 😊

I'm a big fan of the expand_template for that questionable one-define header. Very clean 👍
I do think it'd be better to set CLANG_STATIC_ANALYZER 1 though as we build the static analyzer targets anyways.

Very cool! I actually have a finer grained implementation of this if this would be interesting: https://github.com/eomii/rules_ll/blob/main/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel

This would also allow e.g. the ROCm packages to FORCE_ON hsa and enjoy the benefits of the LLVM configuration 😊

Fun fact I now know that libxml2 also struggles with this 😆

Ah yeah my bad 😅

Does this address the case where we have HSA headers present on the system?

I'll have to agree with @chapuni though, Bazel has exceptional support for bash scripts etc, anything that could be expressed in starlark with genrules and bash scripts seems much better to me than python. E.g. overlaying/moving files is like 10x faster in bash than it is in python.

Thanks, no more issues from my side :) I also think it's a lot better to do this without the subprocess module👍

@GMNGeoffrey From the other conversation: This new syntax may be relevant/useful: https://docs.google.com/document/d/1N81qfCa8oskCk5LqTW-LNthy6EBrDot7bdUsjz6JFC4/edit#heading=h.5mcn15i0e1ch

In D143320#4108554, @GMNGeoffrey wrote:

Ugh that's my bad. I totally forgot about those :-( I think it enforced naming of the llvm-project repo in a way I found unsavory and I never figured out a solution after getting very lost in what Label appears to do vs what I think the documentation says it does...

In D143320#4108196, @GMNGeoffrey wrote:

You'd add -lz as was here previously and let the system handle it. Definitely don't want to reinvent system package search here :-)

Can we use some kind of formatting/linting for this script, potentially at a later time, unrelated to this relocation commit?

In D143351#4107657, @GMNGeoffrey wrote:

Hmm I'm trying to remember this history on this one too. I think this was added after hitting a real bug on Windows... Again @chandlerc

@GMNGeoffrey I originally had a version with a string_flag, but abandoned that because I couldn't solve the following issues:

@MaskRay While we're at it, what do you think about changing zlib to zlib-ng? (https://github.com/zlib-ng/zlib-ng) The original zlib still didn't address https://github.com/madler/zlib/issues/633 and I'd rather spend time on writng a buildfile for zlib-ng than to set -Wno-strict-prototypes (or get spammed by a million warnings).

• Fix typo (zlib -> zstd).
• Remove redundant llvm_zstd_enabled config setting.

Remove redundant llvm_zlib_enabled config setting.

Rebase

Porting to bazel in https://reviews.llvm.org/D143344

• attempt to make patch applicable

@MaskRay It the patch is ok like this, would you mind commiting on my behalf?

Proposing D143320 to address the zlib breakages with recent Clang versions.

• Fix lld:ELF
• Run buildifier on zlib.BUILD

Thanks a lot for the clarification.

@ChuanqiXu I think I only now realized the potential impact of this.

In D136496#3906794, @GMNGeoffrey wrote:

I think this is coming from my complete ignorance of bzlmod, but why exactly do we have a separate llvm-project-overlay repository here? Why isn't it pulling from utils/bazel?

Remove that hacky patch. It is not necessary and will usually be supplied by a bazel registry.

In D137007#3906800, @GMNGeoffrey wrote:

It seems like you're synced to a bad commit that's broken for other reasons. Could you rebase and try the pre-merge checks again?

There is also the situation where @llvm-project-overlay actually refers to the top level directory in the llvm repo. This is the case if we import the module in an external module via bazel_dep(name="@llvm-project-overlay", version="16.0.0-bcr.0"). If we were to disallow builds from within the utils/bazel repo, this may make it possible to keep the labels relative,

Ok I tried a bunch of combinations. Apart from the current one, none seem to work.

A few notes:

In D136496#3892902, @GMNGeoffrey wrote:

I'm not familiar with how bzlmod or the central registry are going to work, but just looking at the content of this patch, I think there is a problem. I think this adds a dependency on exactly the name "llvm-raw" being used when this is imported by external projects. As currently written, it's a relative reference, which has no dependence on this name. I took some care when setting all of this up to not make user-defined strings like that load-bearing, so I think this is undesirable. llvm-raw is kind of an implementation detail name that no one should care about too much though and is unlikely to be referenced by any other dependency, so it isn't too bad to bake it in. It was more important for the name of the final llvm-project Bazel repository where there were already multiple names being used in the wild. The fact that dependency names are user-defined and not necessarily consistent between projects has always been a failing in the Bazel dependency management system though, and it looks like perhaps bzlmod and this registry are attempting to establish canonical names for projects, more aligned with how package managers like pip work. If that's the world we're headed for, then it seems like baking this in may be the right thing to do, but I'm unsure.

Ok I now added the rest of the commits which should give a better picture on how/what I'm doing here 😅

Continuation of https://reviews.llvm.org/D137008.

Continuation of https://reviews.llvm.org/D137007.

Continuation of https://reviews.llvm.org/D136496

@chapuni maybe this here would be better? I can't get this to run, but maybe they work for you with

I can't get arcanist or something else to work. As i don't have commit access, I pushed the changes to this git repo: https://github.com/aaronmondal/llvm-project/commit/652afbe8aff0dbf45d2d07ef5b74fdce137269e3

The way I am implementing these patches should not change anything for the current WORKSPACE build. The patches I will submit will look something like this:

Address comments. Add module.modulemap as well, since it is always exported by the CMake build.

@chapuni I think fully expanding the glob does not really gain us anything. It would only add ~300 lines of headers, which I would assume to not really impact builds anyways, except if a header is missing as in openmp_headers/time.h. Keeping those up to date would likely be unnecessary maintenance burden.

Add missing comma.

In D136452#3880688, @chapuni wrote:

FYI, I intend that builtin_headers_gen may be available for the target for packaging.
Its contents shall be the same (or subset) of the installaed image by CMake.

Address comments.

@csigg Ah yes that was an oversight. The Label()s are redundant.

@fmeum I'm not sure who to tag for this as a reviewer, but I remembered you commenting on both some BCR and LLVM issues. Please refer someone else if you are not the right person for this 😅

Move these headers into glob to clearly differentiate generated and non-generated files.

Well, of course we want to ADD these headers, not exclude them 😅

As I don't have commit access, It'd be nice if someone could commit on my behalf.
Name: Aaron Siddhartha Mondal
Email: aaron@eomii.org

I just noticed that there probably needs to be one change. The doc sometimes describes -fprebuilt-module-interface, which is not a valid option. I think the occurrences were meant to be -fpbrebuilt-module-path.