[PATCH] D50078: clang-format: support aligned nested conditionals formatting

srj added a comment. I think the new formatting is unquestionably better than the old formatting. But... This change means that the output from clang-format-11 can't be made compatible with older versions of clang-format, which is also unfortunate. IMHO it should be a goal to have "breaking" f

[PATCH] D50078: clang-format: support aligned nested conditionals formatting

srj added a comment. > And I also think it should be on by default instead of modifying many > .clang-format files. So IMHO if we add an option, it should be opt-out. I can live with it being opt-out. My big concern is that (as it currently stands) our project gets different 'canonical' clang-f

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

srj added a comment. This is marked as "needs revision", but it's not clear to me what the requested changes actually are. The proposed fix here may not be perfect, but AFAICT, there is simply no good way to exclude headers that are beyond your control at present. Repository: rL LLVM CHANG

[PATCH] D134224: [clang][modules][deps] Report modulemaps describing excluded headers

srj added a comment. This change has broken x86-32 builds, at least on Linux. /usr/local/google/home/srj/GitHub/llvm-project/16/llvm/include/llvm/ADT/PointerIntPair.h:136:25: error: static assertion failed: PointerIntPair with integer size too large for pointer 136 | static_assert(IntB

[PATCH] D134653: [clang][modules] Over-align the `Module` class

srj added a comment. How did the original change pass presubmit testing -- is LLVM no longer testing on 32-bit targets? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134653/new/ https://reviews.llvm.org/D134653 ___

[PATCH] D134653: [clang][modules] Over-align the `Module` class

srj added inline comments. Comment at: clang/include/clang/Basic/Module.h:96 /// Describes a module or submodule. -class Module { +class alignas(8) Module { public: Probably worth a comment here, it's not necessarily obvious to the reader that this is to fix a

[PATCH] D134653: [clang][modules] Over-align the `Module` class

srj added a comment. 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. I'm the one who reporte

[PATCH] D134653: [clang][modules] Over-align the `Module` class

srj added a comment. 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. FYI, adding this patch

[PATCH] D134653: [clang][modules] Over-align the `Module` class

srj added a comment. > Thanks for verifying the build. You can test for correctness by "building" > the `check-clang` target. Thanks -- I'll also build and test Halide locally. > You can contact the infrastructure working group. Note that we run more > configurations post-commit: https://lab.l

[PATCH] D134653: [clang][modules] Over-align the `Module` class

srj added a comment. > Thanks for verifying the build. You can test for correctness by "building" > the `check-clang` target. Tests are passing locally, LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134653/new/ https://reviews.llvm.org/D1346

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. 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 Hal

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. In D141861#4091403 , @jhuber6 wrote: > In D141861#4091383 , @srj wrote: > >> It looks like this change (but not the rG4ce454c654bd >>

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. > 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,

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. In D141861#4091869 , @jhuber6 wrote: > In D141861#4091851 , @srj wrote: > >>> https://github.com/llvm/llvm-project/commit/759dec253695f38a101c74905c819ea47392e515. >>> Does it work if you rev

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. 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. > > Won

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. 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 Window

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. In D141861#4091987 , @jhuber6 wrote: > Can you let me know if adding this fixes it. Unfortunately, no. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141861/new/ https://reviews.llvm.o

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. 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_CROSSCOMPIL

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. In D141861#4092190 , @jhuber6 wrote: > In D141861#4092182 , @srj wrote: > >> In D141861#4092096 , @srj wrote: >> >>> Update: I may have a way to make

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. 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) > a

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. 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 likel

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. In D141861#4094059 , @jhuber6 wrote: > In D141861#4094058 , @srj wrote: > >> In D141861#4094043 , @jhuber6 >> wrote: >> >>> Would this just require c

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. In D141861#4094079 , @jhuber6 wrote: > In D141861#4094063 , @srj wrote: > >> Yes please! > > Let me know if this fixes anything rG9f64fbb882dc >

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. In D141861#4094084 , @srj wrote: > In D141861#4094079 , @jhuber6 wrote: > >> In D141861#4094063 , @srj wrote: >> >>> Yes please! >> >> Let me know if

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. In D141861#4094238 , @srj wrote: > In D141861#4094084 , @srj wrote: > >> In D141861#4094079 , @jhuber6 >> wrote: >> >>> In D141861#4094063

[PATCH] D141051: [CUDA][HIP] Add support for `--offload-arch=native` to CUDA and refactor

srj added a comment. 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 AMDGPU.cpp:773:10: error: could not convert ‘GPUArchs’ from ‘llvm::SmallVector, 1>’ to ‘llvm::Expected > >’ return GPUA

[PATCH] D141861: [nvptx-arch] Dynamically load the CUDA runtime if not found during the build

srj added a comment. 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-inline

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

srj added a comment. > (I certainly agree everyone should be using __atomic and not __sync.) Halide still uses the `__sync` builtins for 32-bit targets, for reasons I don't have at hand now (IIRC there were downstream users with tooling issues, I will re-check). (I don't think we are using `__s

[PATCH] D146987: [Assignment Tracking] Enable by default

srj added a comment. For the record, this also breaks (broke?) Halide: Assertion failed: (!(Elmt.getFragmentOrDefault() == Next.getFragmentOrDefault())), function operator(), file AssignmentTrackingAnalysis.cpp, line 2020. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D146987: [Assignment Tracking] Enable by default

srj added a comment. In D146987#4285299 , @jmorse wrote: > In D146987#4285275 , @srj wrote: > >> For the record, this also breaks (broke?) Halide: >> >> Assertion failed: (!(Elmt.getFragmentOrDefault() == >> Next

[PATCH] D146987: [Assignment Tracking] Enable by default

srj added a comment. >> 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.

[PATCH] D146987: [Assignment Tracking] Enable by default

srj added a comment. 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. ...`std::s

[PATCH] D146987: [Assignment Tracking] Enable by default

srj added a comment. In D146987#4287819 , @jmorse wrote: > Should have been reverted in rG0ba922f60046 > earlier > today, are you still experiencing the same behaviour with that patch

[PATCH] D146987: [Assignment Tracking] Enable by default

srj added a comment. In D146987#4287847 , @srj wrote: > In D146987#4287819 , @jmorse wrote: > >> Should have been reverted in rG0ba922f60046 >>

[PATCH] D100981: Delete le32/le64 targets

srj added a comment. Any chance that this could be (temporarily) reverted? This will break literally all Halide compilation; we're working on a fix on our side, but it would be nice to have a few days to be sure we have it right, and I suspect there's no urgency to removing this right now. Re

[PATCH] D100981: Delete le32/le64 targets

srj added a comment. In D100981#2709205 , @MaskRay wrote: > In D100981#2709044 , @srj wrote: > >> Any chance that this could be (temporarily) reverted? This will break >> literally all Halide compilation; we're w

[PATCH] D100492: [OpenCL] Change OpenCL builtin version encoding

srj added a comment. This change appears to be the injection point for https://bugs.llvm.org/show_bug.cgi?id=50128 -- trying to build clang-tblgen after this change crashes VC2019 if targeting 32-bit Windows (see below). While this is likely an MSVC bug, I presume we still want to support cross

[PATCH] D100492: [OpenCL] Change OpenCL builtin version encoding

srj added a comment. From experimentation, it appears that just pulling the MinVersion and MaxVersion expressions from `BuiltinNameEmitter::EmitBuiltinTable` into separate statements will pacify MSVC, e.g. auto MinVersion = Overload.first->getValueAsDef("MinVersion")->getValueAsInt("ID");

[PATCH] D100492: [OpenCL] Change OpenCL builtin version encoding

srj added a comment. (This is clearly just a weird bug on MSVC's part; there's nothing about the code here that seems obviously unreasonable or complex. Working around compiler bugs is a thing, alas.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D100492: [OpenCL] Change OpenCL builtin version encoding

srj added a comment. In D100492#2717235 , @svenvh wrote: > I am happy to make/accept any adjustments to this patch to fix the particular > build. I don't have access to MSVC though, would you be able to help figure > out what the problematic construct