Skip to content

Don't sign-extend bswap32 in Emit (amd64)#482

Merged
gretay-js merged 1 commit intooxcaml:mainfrom
gretay-js:remove_sign_extend_from_bswap32
Jan 26, 2022
Merged

Don't sign-extend bswap32 in Emit (amd64)#482
gretay-js merged 1 commit intooxcaml:mainfrom
gretay-js:remove_sign_extend_from_bswap32

Conversation

@gretay-js
Copy link
Copy Markdown
Contributor

@gretay-js gretay-js commented Jan 24, 2022

Ibswap operation should treat its input as unsigned, but there is some inconsistently currently. For Ibswap 16, amd64 emits zero-extension, but Ibswap 32 gets sign-extended and we get code that looks like this:

camlT__test_swap32_11:
	subq	$8, %rsp
.L100:
	subq	$24, %r15
	cmpq	8(%r14), %r15
	jb	.L101
.L103:
	leaq	8(%r15), %rbx
	movq	$2303, -8(%rbx)
	movq	caml_int32_ops@GOTPCREL(%rip), %rdi
	movq	%rdi, (%rbx)
	movslq	8(%rax), %rax
	bswap	%eax
	movslq	%eax, %rax                
	movslq	%eax, %rax                ;; and one more time, just to be sure
	movq	%rax, 8(%rbx)
	movq	%rbx, %rax
	addq	$8, %rsp
	ret

because the result of Ibswap 32 is also sign-extended when it's boxed (see Cmm_helpers.box_int_gen), and it's the only way Ibswap 32 is used.

@stedolan I think the sign-extension in Emit became unnecessary after ocaml/ocaml#9006

@xclerc
Copy link
Copy Markdown
Contributor

xclerc commented Jan 25, 2022

Shouldn't we add a test?

@gretay-js
Copy link
Copy Markdown
Contributor Author

gretay-js commented Jan 25, 2022

Shouldn't we add a test?

I am adding a test for it in ocaml_intrinsics library, to go along with other swap32 and Int32unsigned_to_int changes.

@gretay-js gretay-js merged commit 09629e2 into oxcaml:main Jan 26, 2022
mshinwell pushed a commit that referenced this pull request Feb 1, 2022
mshinwell added a commit to mshinwell/oxcaml that referenced this pull request Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants