haxe icon indicating copy to clipboard operation
haxe copied to clipboard

Hashlink: wrong code generation for bit shift + assignment in while loop header

Open MoritzBrueckner opened this issue 3 years ago • 1 comments

When targeting Hashlink, the expression n >>>= 1 does not assign a new value to n if used inside of a while loop, which may cause an endless loop like in the following example (try online, luckily with timeout):

class Test {
	static function main() {
		trace(log2Unsigned(16));
	}
}

@:pure inline function log2Unsigned(n: Int): Int {
	var res = 0;

	while ((n >>>= 1) != 0) {
		res++;
	}

	return res;
}

The generated HL/C code looks similar to this:

r3 = r1;
r4 = 0;
label$8e99391_4_54:
r13 = 1;
r12 = ((unsigned)r3) >> r13; // r12 should be r3, currently each iteration checks for the same unchanged r3
r13 = 0;
if( r12 == r13 ) goto label$8e99391_4_61;
++r4;
goto label$8e99391_4_54;
label$8e99391_4_61:

On other targets this runs without any problems. I'm using Haxe 4.2.5.

MoritzBrueckner avatar Aug 24 '22 19:08 MoritzBrueckner

Looking at the hlopt.txt, it seems to be another case of the hlopt mechanism getting confused. (Similar to #10353) It correctly optimizes out the move into a temp register, but then doesn't handle the move from the temp register back into the original correctly.

	@3   mov 3,0              nop unused                                                     LIVE=0,1
	@4   int 4,@2                                                                            LIVE=0,1
	@5   ushr 3,3,4           ushr 3,0,4                                set n                LIVE=0,1,4
	@6   mov 0,3              nop unused           set n                                     LIVE=1,3
Full hlopt output for the function

``` _Test.Test_Fields_.log2Unsigned@288 ARGS = n:-1

r0  i32       
r1  i32       
r2  void       unused
r3  i32        r2 
r4  i32        r3 
r5  dyn        unused
----- [2] (1)
NEED=	WRITE=1@0
@0   int 1,@0                                                       set res              LIVE=0
@1   mov 1,1              nop mov              set res                                   LIVE=0,1
----- [9,B] (8)
LOOP NEED=0	WRITE=0@6 3@5 4@7
@2   label                                                                               LIVE=0,1
@3   mov 3,0              nop unused                                                     LIVE=0,1
@4   int 4,@2                                                                            LIVE=0,1
@5   ushr 3,3,4           ushr 3,0,4                                set n                LIVE=0,1,4
@6   mov 0,3              nop unused           set n                                     LIVE=1,3
@7   int 4,@0                                                                            LIVE=1,3
@8   jeq 3,4,2                                                                           LIVE=1,3,4
----- [2] (A)
NEED=1	WRITE=1@9
@9   incr 1                                                                              LIVE=0,1
@A   jalways -9                                                                          LIVE=0,1
----- [] (C)
NEED=1	WRITE=3@B
@B   mov 3,1              nop unused                                                     LIVE=1
@C   ret 3                ret 1                                                          LIVE=1
</p>
</details>

Apprentice-Alchemist avatar Aug 25 '22 18:08 Apprentice-Alchemist

@ncannasse This might be something you want to check.

Simn avatar Mar 24 '23 09:03 Simn

I just checked, this is fixed by changes in my merged PR https://github.com/HaxeFoundation/haxe/pull/11461 , the loop node re-computation in hlopt. Could you please check on your side and close the issue if everything is good? @Simn

yuxiaomao avatar Jan 29 '24 14:01 yuxiaomao

I would prefer if we added a test for this before closing.

Simn avatar Jan 29 '24 15:01 Simn

Sure, how can I help to add the test?

yuxiaomao avatar Jan 30 '24 07:01 yuxiaomao

I've committed one, please confirm that this actually tests what we want to test here! And feel free to commit tests like this directly, no need for a PR in such cases.

Simn avatar Jan 30 '24 07:01 Simn

The test code seems good to me, ok thank you for the information!

yuxiaomao avatar Jan 30 '24 07:01 yuxiaomao