Don't use le32/le64#8344
Conversation
le32/le64 has been removed from LLVM19, which is bad for Halide. This attempts to use wasm32/wasm64 targets instead.
|
|
|
Ah, this is the fix for #8333 I guess? |
This reverts commit 09fba5f.
The 64-bit atomics were all on amounts of memory, so a uintptr_t should be safe. One bit of dodginess is that memory_total could possibly exceed 32-bits while there never being more than 32-bits of memory allocated at any one time.
| // On 32-bit platforms we don't want to use 64-bit | ||
| // atomics. Fortunately on these platforms memory usage fits into | ||
| // 32-bit integers. | ||
| sync_compare_max_and_swap((uintptr_t *)(&(instance->funcs[i]).stack_peak), |
There was a problem hiding this comment.
This isn't safe or correct. stack_peak is a uint64, so the result will be incorrect on 32-bit platforms.
There was a problem hiding this comment.
This assumes little-endian. It will just update the low 32 bits of stack_peak. The high bits will be zero on 32-bit targets anyway.
There was a problem hiding this comment.
Ouch.. pretty hacky, needs better documentation. I think the warnings here are a bit of a red herring; I'd like to deal with them but as a separate PR.
There was a problem hiding this comment.
Wouldn't it be cleaner to just change stack_peak to be a uintptr_t?
There was a problem hiding this comment.
I was worried about breaking existing users of the profiler stats.
steven-johnson
changed the title
|
Mac-specific failures appear to be due to a transitory bug in top-of-tree Clang that was improperly coalescing literal strings (!) |
| if (LLVM_PACKAGE_VERSION VERSION_LESS 19.0) | ||
| set(TARGET "le64-unknown-windows-unknown") | ||
| else () | ||
| # TODO: was le64 here, not sure if this is correct or not | ||
| set(TARGET "aarch64-unknown-windows-unknown") | ||
| endif () |
There was a problem hiding this comment.
What prevents us from just using aarch64/arm on all LLVM versions? I'm a bit worried about bugs in Halide 19 that appear only with LLVM 19 and not LLVM 18.
Also, does this introduce a requirement that LLVM be built with ARM and AArch64 backends? We will need to check that, if so. #2282 will rear its head again.
There was a problem hiding this comment.
What prevents us from just using aarch64/arm on all LLVM versions?
Nothing AFAICT. I'll make the change.
Also, does this introduce a requirement that LLVM be built with ARM and AArch64 backends
Yes.
alexreinking
left a comment
alexreinking
left a comment
There was a problem hiding this comment.
LGTM. We have a few alternatives if this change becomes a problem, such as working with package maintainers to include these backends.
|
OK, I think this combination finally works, going to land it |
5 participants
le32/le64 is being removed from LLVM19, which is bad for Halide. This uses i386/x86-64 and wasm32/wasm64 targets instead, with a modification to clang flags.
NOTE that if we go in this direction, it means that LLVM used for building Halide must have WebAssembly codegen enabled.