This change has broken compilation on arm32-linux versions of gcc-7.5; we fail with

In D153587#4644761, @srj wrote:

In D153587#4644755, @ellis wrote:

Thanks for letting me know. I've just pushed a fix at https://github.com/llvm/llvm-project/commit/dfd0cd1cc182789d351953d2c40b8ccd9fdec34f so let me know if it starts building or not

Thanks! Testing now

In D153587#4644755, @ellis wrote:

Thanks for letting me know. I've just pushed a fix at https://github.com/llvm/llvm-project/commit/dfd0cd1cc182789d351953d2c40b8ccd9fdec34f so let me know if it starts building or not

This has injected a build break in an older-but-still-supported version of gcc (at least, I think it's supported): arm-linux-gnueabihf-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0, in which we get failures on lines 62 and 132 of the form

In D156016#4533783, @tianshilei1992 wrote:

I can confirm that 6a684dbc4433a33e5f94fb15c9e378a2408021e0 fixed the build with GCC 7.5.0. Thanks.

In D156016#4533091, @tianshilei1992 wrote:

This patch causes compile error when using GCC 7.5.0.

In D153679#4450286, @nikic wrote:

I think we should revert this change for now and keep the pass until we have NewPM support for the codegen pipeline. @aeubanks What do you think?

In D153679#4450033, @nikic wrote:

@srj Could you please share how you're currently using the pass? It's possible we just have to revert this, but hard to say without code.

Is there any way to get this functionality without the LegacyPassManager?

Hi, I'm trying to update Halide's usage of this API to work properly, there are a few things that aren't clear to me:

The enclosed .ll file will crash llc in the manner I described above. (Prior to this change, it produced a ~500k object file)

This change appears to have inserted an infinite loop in codegen for some Halide targets (well, maybe not infinite, but several minutes, enough for our builder to time out). I'm working on putting together a repro case I can share with you, but in the meantime, a revert would be appreciated to unbreak some downstream code.

In D151782#4406550, @tlively wrote:

Oops, sorry for missing those tests, Caleb. Yes, opening a new review with the fix and tests would be great. Thanks!

Enclosed is a repro case: if I just run llc breakage.ll I fail in the above manner.

FYI, building with an asserts-enabled LLVM, I fail with:

This change has injected a SIGSEGV failure (apparent null pointer deference) in Halide's WebAssembly codegen. I'm working on providing a repro case for you I can post here and hope to have it soon, but in the meantime, can I ask for a revert, to unbreak downstream code?

In D152112#4401561, @paulkirth wrote:

I think this is also causing failures in our CI for the Fuchsia Toolchain. I'm bisecting now to confirm, but it seems most likely given the blamelist + the earlier report. Can you revert this until a fix is ready?

This appears to have broken arm64 codegen for macOS in Halide:

In D146987#4287847, @srj wrote:

In D146987#4287819, @jmorse wrote:

Should have been reverted in rG0ba922f60046 earlier today, are you still experiencing the same behaviour with that patch?

Ah, sorry, I didn't see that -- I'll pull and rebuild this morning to verify!

In D146987#4287819, @jmorse wrote:

Should have been reverted in rG0ba922f60046 earlier today, are you still experiencing the same behaviour with that patch?

In D146987#4287137, @jmorse wrote:

Ah, you're right, it's actually the same assertion message that @srj reported which I totally skipped over. Currently I suspect that's due to different std::sort implementations.

That's a new one -- would you be able to give some context and a reproducer? Thanks for reporting!

Yep -- as of LLVM commit 3c9083f6757cbaf6f8d6c601586d99a11faf642e, Halide is still broken. Working on a repro case now.

In D146987#4285299, @jmorse wrote:

In D146987#4285275, @srj wrote:

For the record, this also breaks (broke?) Halide:

Assertion failed: (!(Elmt.getFragmentOrDefault() == Next.getFragmentOrDefault())), function operator(), file AssignmentTrackingAnalysis.cpp, line 2020.

That's a new one -- would you be able to give some context and a reproducer? Thanks for reporting!

For the record, this also breaks (broke?) Halide:

In D144503#4201050, @srj wrote:

In D144503#4201046, @srj wrote:

Build environment is fairly stock Visual Studio Community 2022 on Windows 10. (I'll go check to see if there are updates that need installing, but they should be pretty close to the latest.)

EDIT: looks like there are some uninstalled updates -- I'll get those installed and re-check, maybe this is just an MSVC bug.

In D144503#4201046, @srj wrote:

Build environment is fairly stock Visual Studio Community 2022 on Windows 10. (I'll go check to see if there are updates that need installing, but they should be pretty close to the latest.)

In D144503#4201043, @kuhar wrote:

Is there some public buildbot that experienced the same failure?

There is also a similar breakage in InstrInfoEmitter.cpp:

According to git bisect, this change appears to have broken llvm-tblgen when building under MSVC

(I certainly agree everyone should be using atomic and not sync.)

In D142279#4120270, @beanz wrote:

LLVM’s C++ interfaces do not provide a stable ABI across all configuration flags (which is not uncommon for C++). As a result llvm-config and the LLVM CMake exports provide the options LLVM was built with for the user.

While it may be true that we could filter some of those options safely, it gets a bit tricky. Options like enabling assertions, or dump methods need to be in sync between the prebuilt LLVM and the user’s library.

In D142279#4117691, @foad wrote:

The warning is reported here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105545

I'm not sure what's best to do. The status quo is that you get lots of compile time warnings. We could add a check for that in the cmake scripts, and disable _GLIBCXX_ASSERTIONS, but then you would silently lose the assertions.

I can reproduce the error above on a debian:bookworm docker using gcc-12 12.2.0-14. But why are you hitting this when building LLVM? Where does -Werror=restrict come from? I don't think we enable that in normal LLVM builds.

This change introduces compilation errors when building in at least some version(s) of gcc/g++; while what we're seeing is clearly a gcc/g++ bug, it would be good to avoid it if possible.

In D139752#4107678, @alexreinking wrote:

This patch might cause problems for other downstreams that (reasonably, but incorrectly) assume that LLVM_DEFINITIONS is a typical CMake list rather than a space-separated list.

In D141861#4094238, @srj wrote:

In D141861#4094084, @srj wrote:

In D141861#4094079, @jhuber6 wrote:

In D141861#4094063, @srj wrote:

Yes please!

Let me know if this fixes anything rG9f64fbb882dc.

Testing now

So far, so good, let me just verify a few more things

Looks good for Halide! Thanks!

In D141861#4094084, @srj wrote:

In D141861#4094079, @jhuber6 wrote:

In D141861#4094063, @srj wrote:

Yes please!

Let me know if this fixes anything rG9f64fbb882dc.

Testing now

In D141861#4094079, @jhuber6 wrote:

In D141861#4094063, @srj wrote:

Yes please!

Let me know if this fixes anything rG9f64fbb882dc.

In D141861#4094059, @jhuber6 wrote:

In D141861#4094058, @srj wrote:

In D141861#4094043, @jhuber6 wrote:

Would this just require checking LLVM_BUILD_32_BITS? Should be an easy change.

I think so. (It might be tempting to check if (CMAKE_SIZEOF_VOID_P EQUAL 8) but LLVM_BUILD_32_BITS is likely to be a more explicit signal.)

SG, want me to push that real quick?

In D141861#4094043, @jhuber6 wrote:

Would this just require checking LLVM_BUILD_32_BITS? Should be an easy change.

In D141861#4092237, @tra wrote:

For what it's worth, NVIDIA has started deprecating 32-bit binaries long ago (https://forums.developer.nvidia.com/t/deprecation-plans-for-32-bit-linux-x86-cuda-toolkit-and-cuda-driver/31356) and the process had finally come to the end with the release of CUDA-12:

In D141861#4092190, @jhuber6 wrote:

In D141861#4092182, @srj wrote:

In D141861#4092096, @srj wrote:

Update: I may have a way to make this work from my side; testing now.

Alas, that didn't work, stlll broken.

Interesting. It's definitely a bad problem that we find the 64-bit version and try to link with it for a cross-compile. I don't want to revert the code because the old FIND_CUDA is officially deprecated. I figured there was surely a way to check if LLVM was cross compiling.

In D141861#4092096, @srj wrote:

Update: I may have a way to make this work from my side; testing now.

In D141861#4092034, @srj wrote:

In D141861#4091987, @jhuber6 wrote:

Can you let me know if adding this fixes it.

Unfortunately, no. (That is: It does not fix it. CMAKE_CROSSCOMPILING is unfortunately a subtle and flaky beast, see e.g. https://gitlab.kitware.com/cmake/cmake/-/issues/21744)

I wonder if this could be a bug in a specific version(s) of CMake? What version(s) have you tried replicating with? Our buildbots are using v3.22.6 (Halide requires a minimum of 3.22).

I am using cmake 3.25.1

In D141861#4091987, @jhuber6 wrote:

Can you let me know if adding this fixes it.

In D141861#4091949, @jhuber6 wrote:

In D141861#4091922, @srj wrote:

Crosscompiling to x86-32 on an x86-64 host doesn't strike me as particularly weird at all (especially on Windows), but apparently it is quite weird for LLVM at this point in time as we keep getting a lot of different things broken there :-)

I'm not very familiar with this type of build. Are there any variables we could pick up to just disable this if it's not building for the host system? Something like CMAKE_CROSSCOMPILING?

In D141861#4091903, @jhuber6 wrote:

In D141861#4091897, @srj wrote:

It's finding a 64-bit CUDAToolkit, which it can't link against because the rest of the build is 32-bit.

Wondering why it didn't find it before then. But that's definitely a weird configuration. Not sure what a good generic solution is. We could always make it dlopen all the time.

In D141861#4091869, @jhuber6 wrote:

In D141861#4091851, @srj wrote:

https://github.com/llvm/llvm-project/commit/759dec253695f38a101c74905c819ea47392e515. Does it work if you revert this? I wouldn't think it wouldn't affect anything. That's the only change that happened after the 16 release as far as I'm aware.

Reverting this (well, actually just monkey-patching in the old code) does indeed make things build correctly again -- a bit surprising to me too, but that's CMake for you. Reverting the change seems appropriate pending a tested fix.

That's bizarre, that means it's finding CUDAToolkit but can't link against it?

https://github.com/llvm/llvm-project/commit/759dec253695f38a101c74905c819ea47392e515. Does it work if you revert this? I wouldn't think it wouldn't affect anything. That's the only change that happened after the 16 release as far as I'm aware.

In D141861#4091403, @jhuber6 wrote:

In D141861#4091383, @srj wrote:

It looks like this change (but not the rG4ce454c654bd) is in the 17 branch, as the latter is now failing in the same way for crosscompiles.

It looks like it's there for me, see https://github.com/llvm/llvm-project/blob/main/clang/tools/nvptx-arch/NVPTXArch.cpp#L20. What is the issue? I made a slight tweak a few days ago on the 17 branch that updated how we could the CUDA driver.

In rG4ce454c654bd4e027fbdcd7b1dba9d164cd36463#1165284, @srj wrote:

Appears to have fixed it, thanks!

In D141861#4060100, @jhuber6 wrote:

In D141861#4060028, @srj wrote:

This change appears to have broken the build when crosscompiling to x86-32 on a Linux x86-64 system; on the Halide buildbots, we now fail at link time with

FAILED: bin/nvptx-arch 
: && /usr/bin/g++-7  -m32 -Wno-psabi -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -Wno-misleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 -DNDEBUG -Wl,-rpath-link,/home/halidenightly/build_bot/worker/llvm-16-x86-32-linux/llvm-build/./lib  -Wl,--gc-sections tools/clang/tools/nvptx-arch/CMakeFiles/nvptx-arch.dir/NVPTXArch.cpp.o -o bin/nvptx-arch  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMSupport.a  -lpthread  -lrt  -ldl  -lpthread  -lm  lib/libLLVMDemangle.a && :
/usr/bin/ld: tools/clang/tools/nvptx-arch/CMakeFiles/nvptx-arch.dir/NVPTXArch.cpp.o: in function `handleError(cudaError_enum)':
NVPTXArch.cpp:(.text._ZL11handleError14cudaError_enum+0x2b): undefined reference to `cuGetErrorString'
/usr/bin/ld: tools/clang/tools/nvptx-arch/CMakeFiles/nvptx-arch.dir/NVPTXArch.cpp.o: in function `main':
NVPTXArch.cpp:(.text.startup.main+0xcf): undefined reference to `cuInit'
/usr/bin/ld: NVPTXArch.cpp:(.text.startup.main+0xf9): undefined reference to `cuDeviceGetCount'
/usr/bin/ld: NVPTXArch.cpp:(.text.startup.main+0x11e): undefined reference to `cuDeviceGet'
/usr/bin/ld: NVPTXArch.cpp:(.text.startup.main+0x131): undefined reference to `cuDeviceGetAttribute'
/usr/bin/ld: NVPTXArch.cpp:(.text.startup.main+0x146): undefined reference to `cuDeviceGetAttribute'
collect2: error: ld returned 1 exit status

I'm guessing that the problem here is that the build machine has Cuda installed (so the headers are found), but no 32-bit version of Cuda (so linking fails).

Probably easy to fix, but as of right now, our 32-bit testing is dead in the water; could someone please revert this pending a proper fix?

Can you let me know if rG4ce454c654bd solves it? I'm guessing the problem is arising when we find the libraries at build configure time but not at build time so we might need another check as well.

In D139752#4089118, @MaskRay wrote:

In D139752#4074965, @raj.khem wrote:

In D139752#4074691, @MaskRay wrote:

@raj.khem would you be able to figure out the issue? :) https://llvm.org/ release/16.x will be created soon...

I have been trying to reproduce it on my end but have not succeeded, I don't have same setup as @srj, I would also look for some alternative ways to do it so we can avoid the issue at hand which seems some weird cmake thing.

@srj It will be nice to figure out the 32-bit glibc Halide issue and fix this for just just-branched-release/16.x so that llvm-project 16.0 can be built with next musl release, otherwise many musl based distributions will have to carry this local patch...

In D141074#4066869, @aeubanks wrote:

this is causing us to get undefined symbol errors for ___floatundisf because we don't link in builtins on win32. not 100% sure on this, but I don't think we're expected to link against builtins on win32

https://bugs.chromium.org/p/chromium/issues/detail?id=1408807

In rGf8d9097168b73e3befd94ac4e16cbc5f944afcec#1165287, @srj wrote:

In rGf8d9097168b73e3befd94ac4e16cbc5f944afcec#1165275, @lebedev.ri wrote:

Reproducer please. That simply means the assertion should be replaced with return undef;.

I'll get you one tomorrow.

In rGf8d9097168b73e3befd94ac4e16cbc5f944afcec#1165275, @lebedev.ri wrote:

Reproducer please. That simply means the assertion should be replaced with return undef;.

Appears to have fixed it, thanks!

This change has injected a failure in Halide's CUDA testing code -- our correctness_predicated_store_load test now fails with

I will attempt to verify this fix later today.

This change appears to have broken the build when crosscompiling to x86-32 on a Linux x86-64 system; on the Halide buildbots, we now fail at link time with

In D139752#4059869, @srj wrote:

From experimentation, it seems that its add_compile_definitions(_FILE_OFFSET_BITS=64) that is the culprit; commenting it out allows us to build and run correctly. (Why it's failing in this way is not at all clear to me, but then, CMake has a few quirks here and there...)

In D139752#4059783, @srj wrote:

-D_FILE_OFFSET_BITS="64 -D_DEBUG -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64

You can use curl -L 'https://reviews.llvm.org/D139752?download=1' | patch -p1 to apply the changes.

Applying this patch fixes my llvm-project build on my Gentoo machine using =sys-libs/musl-9999 (git master). We probably want to fix these issues so that the next release llvm-project 16.0.0 will be buildable with the next release of musl (1.2.4).

Yep, that did the trick, thanks for the quick fix!

FYI, before I could start looking into this, it looks like a more-recent change (https://reviews.llvm.org/D141051) has injected another 32-bit-specific breakage (at least when building under gcc-7) -- I'm investigating that first :-/

For reasons that aren't yet clear to me, this change is failing to compile when using gcc-7 and targeting 32-bit targets; the error is of the form

In D139752#4045548, @MaskRay wrote:

@srj Will you be able to help figure out the 32-bit Halide issue? Thanks in advance:)

In D139752#4008409, @MaskRay wrote:

In D139752#4008370, @srj wrote:

This change continues to break all 32-bit builds of Halide (and probably anything else that consumes LLVM via CMake); please revert ASAP until a fix can be found.

I applied @alexreinking's suggestion. Feel free to revert if errors still continue.

This change continues to break all 32-bit builds of Halide (and probably anything else that consumes LLVM via CMake); please revert ASAP until a fix can be found.

This change seems to have injected a failure into Halide builds on 32-bit Linux: we now fail with <command-line>:0:70: error: token "=" is not valid in preprocessor expressions; examination of our CMake-based build command line shows that it is something like:

This fixes our issue -- thanks!

Sorry, but I think I have another codegen error, but this one is more likely to be legit, since the unit test for it is producing incorrect results after this change (but not before).

In rG073d5e5945c428e20db0884943e6dcb7ff2158df#1149589, @kparzysz wrote:

Hi Steven,
Thank you for the testcase, it's important to investigate the differences even if it turns out that they're expected, so this is not a problem at all.

I have verified that in this case the changes in the output are actually expected, and the new output should perform better than the old. There are two different shuffles in the testcase that are now handled differently, these two small testcases reproduce these situations:

Before this change, we got vdelta(*,*) instructions generated; afterwards, we don't.

Hi, this seems to have injected a regression in Halide's codegen: LLVM no longer produces vdelta in all of the situations it did before. Let me try to get you a repro case.

Update: confirmed that https://reviews.llvm.org/rG17dcbd8165479d5b2d7f827bfcb271b50ee03872 appears to fix the injection in Halide

Looks like the Halide failure is a SIGFPE on an idiv instruction... we are doing idiv %r15b with edx=0xff, eax=0x8382ff80, r15=0xffffffff... i.e., we are dividing by -1. The result won't fit into 32 bits, so we fail.

Thanks for verifying the build. You can test for correctness by "building" the check-clang target.

Thanks for verifying the build. You can test for correctness by "building" the check-clang target.

In D134653#3815627, @jansvoboda11 wrote:

Pre-commit CI only runs on x64 Debian and x64 Windows. I don't have good way to reproduce the issue, so ideally the person who reported it (@ms178?) steps up.

In D134653#3815627, @jansvoboda11 wrote:

Pre-commit CI only runs on x64 Debian and x64 Windows. I don't have good way to reproduce the issue, so ideally the person who reported it (@ms178?) steps up.

How did the original change pass presubmit testing -- is LLVM no longer testing on 32-bit targets?

This change has broken x86-32 builds, at least on Linux.

Fixes the Halide crash, thanks!

The line numbers in the ASAN trace are slightly different from this patch, but the culprit seems to be:

OK, I finally got a failure with ASAN enabled, hopefully this will be enough to track it down:

Running in a different environment with a bit more runtime checking gives me this:

Slightly more detailed traceback from a RelWithDebInfo build of LLVM. Looks like one of the SmallVectors created during processing of case Instruction::InsertElement is bad when we try to free it? I wonder if Mask.swap(PrevMask); on SLPVectorizer.cpp:8160 could be suspicious here.

In D133891#3803631, @ABataev wrote:

You can use -mllvm -opt-bisect-limit to limit number of transformation passes

As of yet, I haven't been unable to get a .ll file that will repro this -- we crash while generating it, and AFAIK there isn't a way to defer running the SLP pass until llc -- so currently the simplest repro case requires building Halide locally. In the meantime, here's a stacktrace of the failure:

This change seems to have injected a bad-call-to-free() crash in some Halide code. Looking to get a good repro case for you now.

OK, here is a simple case that repros it reliably with the original patch in place:

In rG16e5d6d7f98f1119aab3d10ec4f9e59b5aacd359#1124229, @mizvekov wrote:

@srj any chance this could actually be related to https://github.com/llvm/llvm-project/issues/55686 instead, and this patch just simply exposed it in some other scenario?

OK, so the issue here appears to be that the failing section of code is relying on using C99 Variable Length Arrays:

This change has injected a build breakage for MSVC builds: