git-clang-format

The sema portions of this change are still causing an issue. Although the revert (rGf95bd18b5faa6a5af4b5786312c373c5b2dce687) and the subsequent re-application in rG3466e00716e12e32fdb100e3fcfca5c2b3e8d784 addressed the debug info issue, the Sema issue remains.

This seems reasonable to me. I think that a bit more exposition in the commit message is warranted though: explain why/how they are useful (i.e. they can be useful to validating the driver behaviour for path handling).

I wonder if we can just pay a penalty and simplify this to always just check the first member unconditionally.

Thanks!

Happy to help, thanks to Howard for the many years of stewardship!

Sure, the argument of the cache hits is definitely reasonable - I'm not a fan of the naming, but can understand the desire to not rename all the variables across all the projects.

In D77912#3465615, @tonic wrote:

My apologies for missing that. I will be creating a patch to remove this then and other references.

I believe that the comment may be referring to the algorithm rather than the implementation itself as the book provides assembly listings for PPC.

The LLVM Foundation legal counsel has advised us that it violates copyright.

In D77912#3465615, @tonic wrote:

In D77912#3465553, @MaskRay wrote:

In D77912#3465400, @tonic wrote:

Unfortunately, this patch violates copyright of the The PowerPC Compiler Writer's Guide. Please revert this patch, remove the copyrighted code, and you can submit a new patch for the other changes. For future patches that use guides with copyright, please consult with the LLVM Foundation board before committing. Thank you!

If this request is related to "// Translated from Figure 3-40 of The PowerPC Compiler Writer's Guide" in the code, then I think it should apply to the very first commit
fd089990f76ec6392dbd0138ab874d90c632d2a4 (2009) by @ddunbar .

This patch merely copied this old comment. I don't think I am the appropriate one to take any action here.

Please also read the nature of the patch :)

My apologies for missing that. I will be creating a patch to remove this then and other references.

I don't have an example module sadly. It was something that I ran into with Swift code import the WinSDK module defined in https://github.com/apple/swift/blob/main/stdlib/public/Platform/winsdk.modulemap.

This was something that I was hitting an issue with. In particular, it was a module build for a module which pulled in intsafe.h. Now, given that it is a preprocessor macro, it would stand to reason that it will normally be dropped and thus won't matter if the frontend doesn't actually support a 128-bit type. However, with a module, we would attempt to process the expression. I do admit it is on a slightly more tenuous ground as Microsoft may somehow do something different.

Seems reasonable, though I'm not a fan of the variable names - they seem a bit difficult to read due to no separation (e.g., %fs-src-root or %fs_src_root vs %fssrcroot)

In D121510#3377285, @akyrtzi wrote:

@compnerd the windows bot says

[110/8000] Building ASM object lib\Support\CMakeFiles\LLVMSupport.dir\BLAKE3\blake3_avx512_x86-64_windows_msvc.asm.obj
Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30136 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

cl : Command line warning D9035 : option 'o' has been deprecated and will be removed in a future release
cl : Command line warning D9024 : unrecognized source file type 'C:\ws\w32-1\llvm-project\premerge-checks\llvm\lib\Support\BLAKE3\blake3_avx512_x86-64_windows_msvc.asm', object file assumed
cl : Command line warning D9027 : source file 'C:\ws\w32-1\llvm-project\premerge-checks\llvm\lib\Support\BLAKE3\blake3_avx512_x86-64_windows_msvc.asm' ignored
cl : Command line warning D9021 : no action performed

Do you have a hint what is going wrong, is the .asm file extension not correct for passing them to the compiler?

I believe that the break after ( in CMake is counter the style of the rest of the project. I'd recommend against it.

I'd like to see at least https://reviews.llvm.org/D108416#inline-1146215, https://reviews.llvm.org/D108416#inline-1146214, and https://reviews.llvm.org/D108416#inline-1146209 addressed pre-commit. Would be nice to have the other ones resolved as well, but they are more stylistic.

Using the TARGET_OBJECTS: generator seems better. However, I also have concerns over using the shared library objects. At least for Windows, reuse of the objects meant for a shared object is not really correct. This is non-portable at best, and incorrect at worse. I would rather that we just introduce a proper target that we use can for embedding into another library (which may or may not expose its interfaces from the library that integrates it).

Thanks @thakis - 906e60b9f923464cba0f71a9205846550752162f should have addressed that from a few days ago (sorry about not mentioning that here)

I think that this is almost correct now. While this will generate the binaries correctly, this is still a problem for tests. Without the absolute path, we now run the risk of the DT_NEEDED being correct but without a DT_RPATH/DR_RUNPATH, this could pick up the wrong library still at runtime. While the link time issue is resolved, the runtime portion is still a problem.

Add missing null-terminators.

Fix build rules

This is a more complete implementation that allows for error reporting and use of UCS-2 as that is needed for the search path alteration.

I'm not sure that this is a good idea. This now means that you can link against two different versions of this library - one when building LLVMSupport and one that any user linking against LLVMSupport happens to find in the library search path. Consider someone explicitly providing a path to the terminfo library when configuring. At the very least, the library search path should be propagated to ensure that the right version is used.

@craig.topper - ping

In D112890#3099256, @tschuett wrote:

If I understand you correctly, I would need to pass something ala -target riscv-xx to enable __riscv_vector. However, this is impossible because the risk target is disabled. So I thing it is save what you are doing.

In D112890#3099208, @tschuett wrote:

I am undecided between this may break something and I cannot use the risc-v headers on an x86 machine.

@mstorsjo I think that it might be interesting to consider a wrapper for enable_languages and to split that out from project.

I think that as long as the builders remain green, this is fine. Yes, this is very likely to break users. We should announce this change on llvm-dev at the very least, and you *could* possibly add a transition helper in the form of:

I think that we might really want to subsequently rework the compiler-rt handling to use target_compile_options. This munging of flags manually is really rather onerous, but that is beyond the scope of this change.

While I think that the direction of this change is amazing, I'm less enthusiastic about the naming. I think that it is particularly confusing for the flags to use C and CXX as the prefixes for the flags as we have llvm-libc (aka "c") and libc++ (aka "c++", "cxx"). Using a more verbose name of COMPILER_ or C_COMPILER_, CXX_COMPILER_ would allow the flags to be easily attributed to the compiler or the runtime. However, the C standard flags definitely should be dropped.

@thakis - thanks, seems that I had a part of the change sitting in my stash ... I had added a -NOT to verify the behaviour, and forgot to remove it. I'll revert the revert with the fix.

@Eugene.Zelenko - sorry, I didn't see the additional comments before the commit. I'm happy to do a follow up depending on the resolution.

Address review feedback

In D109192#2990039, @echristo wrote:

Hi Saleem,

I think I understand where you're going, but I think this deserves a writeup and consideration on llvm-dev with your desired endpoint for the source base and what changes to both current and future development will need to happen with some benefits and costs outlined. Might want to include a "support story" for the project along these lines as well - or at least be clear that this doesn't change or affect what long term APIs we are willing to support.

Thoughts?

As to the comment on the consistency - there are a number of different layouts that are inconsistent with each other :-(. This is messy, and therefore forcing the user to specify the paths seems like a good compromise.

I really would prefer to avoid adding bin/<TRIPLE> to the default installation path. This doesn't match the defaults for most platforms (e.g. exherbo uses /usr/<triple>/bin - which would be a default value of ${LLVM_DEFAULT_TARGET_TRIPLE}/bin. On Windows, it seems that bin, bin64, bin64a is the style that Microsoft uses. Having to have different defaults seems unfortunate. Defaulting to bin and letting the user override it seems reasonable to me.