- User Since
- Aug 27 2022, 4:49 AM (30 w, 3 d)
Rerun against tests. Previous failure was most likely unrelated to this change,
but let's make sure.
Mon, Mar 20
Sun, Mar 12
Wed, Mar 8
Mon, Mar 6
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.
Sun, Mar 5
Sat, Mar 4
Fri, Mar 3
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.
Wed, Mar 1
Tue, Feb 28
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.
Feb 22 2023
Feb 16 2023
Feb 15 2023
My bad, thanks!
Feb 14 2023
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?
Feb 13 2023
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.
Feb 12 2023
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 😊
Feb 11 2023
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
Feb 10 2023
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?
Feb 7 2023
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👍
Feb 6 2023
@GMNGeoffrey From the other conversation: This new syntax may be relevant/useful: https://docs.google.com/document/d/1N81qfCa8oskCk5LqTW-LNthy6EBrDot7bdUsjz6JFC4/edit#heading=h.5mcn15i0e1ch
Can we use some kind of formatting/linting for this script, potentially at a later time, unrelated to this relocation commit?
@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).
Feb 5 2023
- Fix typo (zlib -> zstd).
- Remove redundant llvm_zstd_enabled config setting.
Remove redundant llvm_zlib_enabled config setting.
Porting to bazel in https://reviews.llvm.org/D143344
- attempt to make patch applicable
Feb 4 2023
@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
Dec 9 2022
Thanks a lot for the clarification.
Dec 8 2022
@ChuanqiXu I think I only now realized the potential impact of this.
Nov 4 2022
Remove that hacky patch. It is not necessary and will usually be supplied by a bazel registry.
Oct 29 2022
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.
Oct 28 2022
A few notes:
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
Oct 26 2022
@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
Oct 25 2022
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.
Oct 24 2022
Add missing comma.
Oct 21 2022
@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
Aug 30 2022
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.