Odin icon indicating copy to clipboard operation
Odin copied to clipboard

Support LLVM 18 (non-windows targets for now)

Open laytan opened this issue 1 year ago • 13 comments

~~Very much still a draft~~, needs a bunch of testing on all the targets and on earlier version to make sure changes to support llvm 18 don't break older versions.

To test / to do:

  • [x] Darwin
    • [x] amd64
      • [x] llvm-18
      • [x] clang-18 (linking)
      • [x] older versions still work
      • [x] Successfully runs CI/all tests with and without optimisations and -sanitize:address
        • [x] test_core_image.run_png_suite:1507 (normal failure) (on a few specific tests in the suite with messages similar to tbyn3p08 test 3 hash is c82111f1, expected b1d45c1e with Options{alpha_add_if_missing, blend_background} - not caused by this upgrade, made #3480
        • [x] All other tests
    • [x] arm64
      • [x] llvm-18
      • [x] clang-18 (linking)
      • [x] older versions still work
      • [x] Successfully runs CI/all tests with and without optimisations and -sanitize:address
  • [x] Linux
    • [x] amd64
      • [x] llvm-18
      • [x] clang-18
      • [x] older versions still work
        • [x] 17
        • [x] 14
        • [x] ...
      • [x] Successfully runs CI/all tests with and without optimisations and -sanitize:address
        • [x] test_core_hash.test_xxhash_vectors:323 (segfault) (something about 128 bit ints in relation with packed structs is going wrong)
        • [x] test_core_image.run_png_suite:1507 (normal failure) (on a few specific tests in the suite with messages similar to tbyn3p08 test 3 hash is c82111f1, expected b1d45c1e with Options{alpha_add_if_missing, blend_background} - not caused by this upgrade, made #3480
  • [x] WASM - Tested manually and by running https://github.com/colrdavidson/spall-web
  • [x] FreeBSD - Tested by Discord user siemato

~~- [ ] OpenBSD~~ ~~- [ ] Haiku~~ ~~- [ ] Essence~~ ~~- [ ] Linux i368~~ ~~- [ ] Linux arm64~~ ~~- [ ] Linux arm32~~

Striked through targets are targets I don't intend to test, I don't own these, feel free to test and let me know and I will update here

laytan avatar Mar 21 '24 21:03 laytan

Interesting test failure, must have to do with incrementing the minimum alignment? All the other changes are only applied if llvm 18 is used and CI is still on 17 here.

laytan avatar Apr 16 '24 17:04 laytan

Works on Windows. An added nicety is that address sanitizer was fixed on windows in llvm 18.

gb_global TargetMetrics target_linux_amd64 = {
	TargetOs_linux,
	TargetArch_amd64,
	8, 8, 16, 16,

needs to be carried over to windows_amd64 otherwise there is an access-violation accessing a u128 in math/big, which was found by asan running the demo.

With that fix, the demo runs clean as a whistle with and without all optimization modes with and without -debug.

JesseRMeyer avatar Apr 16 '24 20:04 JesseRMeyer

Tried this out with LLVM 18.1.4 compiled from source on Arch (btw).

  • There appears to be a regression or change in how llvm-config needs to be invoked if the .a files are missing (at least I needed to add --link-shared to the LDFLAGS invocation). This may be LLVM bug 83698, but see the note about my screwed up LLVM build.
  • tests/core/hash (make hash_test) fails with the default -o:speed, passes with the optimization disabled.
  • The other core tests appear to pass.

Note: My LLVM build is held together by duct-tape, string, and tears (tools/llvm-shlib/typeids.test and only that test fails when trying to run the regression tests with ninja check), and is based on the old llvm17 AUR package.

Yawning avatar Apr 23 '24 11:04 Yawning

Yep I am going to look at the test failures on Linux with optimizations, if you enable optimizations on all the tests it currently fails on:

  • test_core_hash.test_xxhash_vectors:323 (segfault) (something about 128 bit ints in relation with packed structs is going wrong)
  • test_core_image.run_png_suite:1507 (normal failure) (on a few specific tests in the suite with messages similar to tbyn3p08 test 3 hash is c82111f1, expected b1d45c1e with Options{alpha_add_if_missing, blend_background}

laytan avatar Apr 23 '24 19:04 laytan

These test failures are real head scratchers, only happening on optimised builds so it is hard to debug, I have ruled out that it is sroa by turning it off and it still fails. Also, doing the slightest change in specific places makes it work or not work, ie printing out a value or putting in a runtime.trap() might cause it not to fail anymore.

Also, in the image failure, it is just giving the wrong output, there is no crash or anything, and with the image loading proc being so large it is hard to pin down.

For the image tests I have created a separate issue, this also fails on LLVM 17 so it is not caused by this upgrade, and I haven't figured it out in reasonable time. There is even a comment in the test file here https://github.com/odin-lang/Odin/blob/c72a269b7cfd5f955254f9c3ad8e5e63bac7aa2c/tests/core/image/test_core_image.odin#L1494 by @Kelimion mentioning the failure.

I got the hash test segfault down to this reproduction (by trial and error, the slightest changes "fix" it) at the moment and will try to fix it:

package main

import "core:fmt"
import "base:runtime"

My_Any :: struct {
	it: ^u128,
	id: int,
}

main :: proc() {
	v := XXHASH_Test_Vectors_With_Secret { // Length: 000
		/* XXH3_64_with_secret  */ 0x59b41c3adac0be46,
		/* XXH3_128_with_secret */ 0x7553b6679bde5657212be7305b49ae75,
	}

	print(My_Any{&v.xxh3_128_secret, 6})

	e := v.xxh3_128_secret
	print(My_Any{ &e, 5 })
}

print :: #force_no_inline proc(x: $T) {
	fmt.println(x)
}

XXHASH_Test_Vectors_With_Secret :: struct #packed {
	/*
		With Custom Secret
	*/
	xxh3_64_secret:  u64,
	xxh3_128_secret: u128,
}

laytan avatar Apr 24 '24 18:04 laytan

Fixed the last test failure, there was an unaligned load because of the packed struct field being loaded on the types original alignment, we know track if a type is from a packed struct and set the alignment of loads to 1.

laytan avatar Apr 25 '24 15:04 laytan

~~This PR is now ready to review/merge~~

laytan avatar Apr 25 '24 15:04 laytan

Windows is technically supported here too, @JesseRMeyer has been test driving that this week. It just requires updating the dll and headers when we want to. Added benefit of updating to 18 on Windows is that they (llvm) fixed the address sanitiser.

laytan avatar Apr 25 '24 15:04 laytan

FreeBSD amd64 has been tested by discord user siemato and confirmed to work on his projects

laytan avatar Apr 26 '24 20:04 laytan

Only thing I did was rebase and now the Linux tests fail

laytan avatar Apr 28 '24 13:04 laytan

Can't reproduce it locally either

laytan avatar Apr 28 '24 13:04 laytan

~~Seems to be a problem with the new string map, reverting 5969796fbfaa62ea6aba8f0e3915ab8282bb71b5 fixes this~~ Edit: looks like it fixed the issue here by accident, but there is an actual issue here

laytan avatar Apr 28 '24 13:04 laytan

Oke put in a better fix to the packed struct loads that fixes the Linux failure and is a much neater fix.

So once again, and now for real, this is ready to go :)

laytan avatar May 02 '24 17:05 laytan