Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hl] add element type to HArray #11734

Merged
merged 5 commits into from
Aug 1, 2024
Merged

Conversation

Simn
Copy link
Member

@Simn Simn commented Jul 26, 2024

This changes HL's HArray to carry an element type, which I want to use to deal with the problems described in #11568.

At the moment we're getting some hl-check failures in the unit tests that all look like this:

  78 |   eq(arr[0].high, 0);
     |      ^^^
     | Check failure at fun@1660 @1B - array(dyn) should be array(i64)

 ERROR  src/unit/TestInt64.hx:79: characters 6-9

  79 |   eq(arr[0].low, 1);
     |      ^^^
     | Check failure at fun@1660 @35 - array(dyn) should be array(i64)

I suspect that there's some bad typing going on here involving unapplied type parameters, which lead to the unwanted array(dyn).

Compiling a standalone

function main() {
	var a = new haxe.atomic.AtomicInt(0);
}

on the other hand gives some very suspicious errors like these:

 ERROR  C:\git\haxe\std/hl/_std/String.hx:101: characters 13-15

 101 |   var out = [];
     |             ^^
     | Check failure at fun@7 @2 - array(i32) should be array(String)

 ERROR  C:\git\haxe\std/hl/_std/haxe/NativeStackTrace.hx:23: characters 29-34

  23 |   var arr = new NativeArray(count);
     |                             ^^^^^
     | Check failure at fun@258 @3 - array(i32) should be array(abstract(hl_symbol))

I currently have no idea where the array(i32) comes from. The dump looks normal:

	public static function exceptionStack[Function:() -> hl.NativeArray<haxe.Symbol>]
		[Block:Dynamic]
			[Var count<0>(VUsedByTyper):Int]
				[Call:Int]
					[Field:(arr : hl.NativeArray<haxe.Symbol>) -> Int]
						[TypeExpr haxe.NativeStackTrace:Class<haxe.NativeStackTrace>]
						[FStatic:(arr : hl.NativeArray<haxe.Symbol>) -> Int]
							haxe.NativeStackTrace
							exceptionStackRaw:(arr : hl.NativeArray<haxe.Symbol>) -> Int
					[Const:hl.NativeArray<haxe.Symbol>] null
			[Var arr<0>(VUsedByTyper):hl.NativeArray<haxe.Symbol>]
				[Call:hl.NativeArray<haxe.Symbol>]
					[Ident:Int -> Unknown<0>] $aalloc
					[Local count(0):Int:Int]
			[Call:Int]
				[Field:(arr : hl.NativeArray<haxe.Symbol>) -> Int]
					[TypeExpr haxe.NativeStackTrace:Class<haxe.NativeStackTrace>]
					[FStatic:(arr : hl.NativeArray<haxe.Symbol>) -> Int]
						haxe.NativeStackTrace
						exceptionStackRaw:(arr : hl.NativeArray<haxe.Symbol>) -> Int
				[Local arr(0):hl.NativeArray<haxe.Symbol>:hl.NativeArray<haxe.Symbol>]
			[Return:Dynamic] [Local arr(0):hl.NativeArray<haxe.Symbol>:hl.NativeArray<haxe.Symbol>]

Note how all occurrences of hl.NativeArray have haxe.Symbol as type parameter. This makes me think that there's an unrelated bug in genhl here.

@Apprentice-Alchemist
Copy link
Contributor

The problem is that the function returned by alloc_std ctx "alloc_array" [HType;HI32] (HArray et) is always the same and thus has the return type passed to the first invocation (this is expected behavior and not really a bug).
This then makes hl-check fail here:

check tret (rtype r)

@Simn
Copy link
Member Author

Simn commented Jul 31, 2024

Ah right, there's a lookup involved... thank you for pointing me in the right direction! For now I've fixed this by adding an OUnsafeCast to the parameterized array type which I think is how these things should work.

The failures in TestInt64 remain and are likely going to be more of a headache.

@Apprentice-Alchemist
Copy link
Contributor

Looks like it doesn't like arrays of Int64.
Array<Int64> becomes hl.types.ArrayObj (which wraps a NativeArray<Dynamic>), but when doing array access it tries to store the inner array in a register of type NativeArray<Int64>.

haxe/src/generators/genhl.ml

Lines 3160 to 3168 in d033cdb

let arr = alloc_tmp ctx (HArray vt) in
op ctx (OField (arr,ra,1));
let r = alloc_tmp ctx at in
op ctx (OGetArray (r,arr,ridx));
hold ctx arr;
let r = f r in
free ctx arr;
op ctx (OSetArray (arr,ridx,r));
r

The solution is probably to turn Array<Int64> into ArrayBytes<Int64> instead of ArrayObj.

* [hl] Add specialized Array implementation for I64.

* [hlc] Fix HArray type generation.
@Simn Simn marked this pull request as ready for review July 31, 2024 13:17
@Simn
Copy link
Member Author

Simn commented Jul 31, 2024

@yuxiaomao Checking this one would be good as well. I'm not very confident regarding anything HL-related.

@yuxiaomao
Copy link
Contributor

 54 |   o.array.blit(0, array, 0, length);
    |   ^^^^^^^
    | Don't know how to cast array(f64) to array(dyn)

o.array and array has type hl.NativeArray<T>

@Simn
Copy link
Member Author

Simn commented Jul 31, 2024

I've factored out all occurrences of "alloc_array" to avoid similar problems like that $aalloc one we identified. If that doesn't help then I'll likely need some more context for the failure(s).

@Apprentice-Alchemist
Copy link
Contributor

Here's a repro:

import hl.NativeArray;

function main() {
	var a = new hl.NativeArray<Float>(1);
	a[0] = 0.0;
	var b:NativeArray<Float> = new hl.NativeArray<Float>(1);
	b[0] = 1.0;
	foo(a, b, 1);
	trace(a[0]);
}

The problem here is that hl.NativeArray.blit ends up having type (this:NativeArray<Dynamic>, src:NativeArray<Dynamic>, srcPos:Int, srcLen:Int):Void, so you can't call it on/with a NativeArray<Float>.

A solution is to wrap blit like this:

@:hlNative("std", "array_blit") static function real_blit(dest:NativeArray<Any>, pos:Int, src:NativeArray<Any>, srcPos:Int, srcLen:Int):Void {}

public inline function blit(pos:Int, src:NativeArray<T>, srcPos:Int, srcLen:Int):Void {
    real_blit(cast this, pos, cast src, srcPos, srcLen);
}

@Simn
Copy link
Member Author

Simn commented Jul 31, 2024

Thanks! That does look a bit funky but it indeed seems to work.

@yuxiaomao
Copy link
Contributor

Seems good now for Shiro's project.

@Simn
Copy link
Member Author

Simn commented Aug 1, 2024

Thank you for checking!

@Simn Simn merged commit 8b18a2f into development Aug 1, 2024
98 of 99 checks passed
@Simn Simn deleted the hl_harray_with_element_type branch August 1, 2024 07:41
@yuxiaomao
Copy link
Contributor

Seems causing hl/jit cmake 32bit on windows failed (that's very wired and I don't understand).
https://github.com/HaxeFoundation/hashlink/actions/runs/10195243400
Previous run on master was based on last commit which works fine, I also run actions on my fork yesterday and it worked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants