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

parser: allow lock prefix instructions and numbered reg in asm inline #21022

Merged
merged 14 commits into from Mar 16, 2024

Conversation

felipensp
Copy link
Member

@felipensp felipensp commented Mar 14, 2024

Allow asm inline cases like:

__asm__ volatile("lock orl $0, (%esp)");
__asm__ volatile("lock orq $0, (%rsp)");
	$if i386 {
		asm i386 {
			lock orl '(%esp)', 0
		}
	} $else $if amd64 {
		asm amd64 {
			lock orq '(%rsp)', 0
		}
        }
		asm amd64 {
			lock cmpxchgb '%1', '%2'
			; =a (rv)
			+m (*&u8(atom))
			; q (xchg)
			0 (cmp)
			; memory
		}

@felipensp felipensp changed the title parser: allow lock prefix instructions on asm inline parser: allow lock prefix instructions and numbered arg in asm inline Mar 14, 2024
@felipensp felipensp marked this pull request as ready for review March 14, 2024 19:59
@felipensp felipensp changed the title parser: allow lock prefix instructions and numbered arg in asm inline parser: allow lock prefix instructions and numbered reg in asm inline Mar 14, 2024
@spytheman
Copy link
Member

Please add tests in vlib/v/slow_tests/assembly/asm_test.amd64.v.

vlib/v/parser/parser.v Outdated Show resolved Hide resolved
Comment on lines 1767 to 1776
p.check(.name)
if p.tok.kind == .number {
p.check(.number)
} else {
p.check(.name)
}
Copy link
Member

@spytheman spytheman Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? What syntax will trigger the number check?

Copy link
Member

@spytheman spytheman Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add either a test, or an inline code comment here in the parser source, with the concrete syntax that it allows, or both.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is about the constraint part.

image

@spytheman spytheman marked this pull request as draft March 15, 2024 07:21
@spytheman
Copy link
Member

This sample:

fn main() {
        asm amd64 {
                lock orq (%rsp), 0
        }
}

does not compile cleanly on both master, and with this PR.

#0 09:22:35 ᛋ fix_asm_inline /v/oo❱git co master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
#0 09:22:36 ᛋ master /v/oo❱./v self
V self compiling ...
V built successfully as executable "v".
#0 09:22:40 ᛋ master /v/oo❱cat a.v
fn main() {
        asm amd64 {
                lock orq (%rsp), 0
        }
}
#0 09:22:44 ᛋ master /v/oo❱./v a.v
a.v:3:12: error: unexpected token `(`, expecting name
    1 | fn main() {
    2 |     asm amd64 {
    3 |         lock orq (%rsp), 0
      |                  ^
    4 |     }
    5 | }
#1 09:22:46 ᛋ master /v/oo❱git switch -
Switched to branch 'fix_asm_inline'
#0 09:22:49 ᛋ fix_asm_inline /v/oo❱./v self
V self compiling ...
V built successfully as executable "v".
#0 09:22:53 ᛋ fix_asm_inline /v/oo❱./v a.v
a.v:3:12: error: invalid token in assembly block
    1 | fn main() {
    2 |     asm amd64 {
    3 |         lock orq (%rsp), 0
      |                  ^
    4 |     }
    5 | }
#1 09:22:56 ᛋ fix_asm_inline /v/oo❱

Only the parser error is different with this PR.

@spytheman
Copy link
Member

spytheman commented Mar 15, 2024

This compiles cleanly with this PR 🥳 , but cgen errors on master:

fn main() {
        asm amd64 {
                lock inc eax
        }
}

@spytheman
Copy link
Member

spytheman commented Mar 15, 2024

This is a parser error on master, but it is fine here: asm amd64 { lock orq rsp, 0 }.

Later with tcc, it generates code that leads to a runtime error:

/tmp/v_1000/x.01HS0GH0PXCQ6FMGR3QCGPNPK7.tmp.c:12951: at main__main: RUNTIME ERROR: illegal instruction

With gcc, it is a C compiler error:

/tmp/v_1000/x.01HS0GHPANSP1XEQER4TFYXJDY.tmp.c:12976: Error: expecting lockable instruction after `lock'

since it generated:

void main__main(void) {
    __asm__ (
        "lock orq $0, %rsp;"
    );
}

and apparently gcc does check the inline assembly.

@felipensp
Copy link
Member Author

This is a parser error on master, but it is fine here: asm amd64 { lock orq rsp, 0 }.

Later with tcc, it generates code that leads to a runtime error:

/tmp/v_1000/x.01HS0GH0PXCQ6FMGR3QCGPNPK7.tmp.c:12951: at main__main: RUNTIME ERROR: illegal instruction

With gcc, it is a C compiler error:

/tmp/v_1000/x.01HS0GHPANSP1XEQER4TFYXJDY.tmp.c:12976: Error: expecting lockable instruction after `lock'

since it generated:

void main__main(void) {
    __asm__ (
        "lock orq $0, %rsp;"
    );
}

and apparently gcc does check the inline assembly.

Oh sorry. I missed to update the PR description. vfmt just showed me the proper syntax for the (%reg) stuff, so I have declined some changes on the PR and so I focused on the numbered constraint and lock-prefix.

The correct test case for the PR is:

	$if amd64 {
		asm amd64 {
			lock cmpxchgb '%1', '%2'
			; =a (rv)
			+m (*&u8(atom))
			; q (xchg)
			0 (cmp)
			; memory
		}
	}

@felipensp
Copy link
Member Author

And:

	$if i386 {
		asm i386 {
			lock orl '(%esp)', 0
		}
	} $else $if amd64 {
		asm amd64 {
			lock orq '(%rsp)', 0
		}
        }

@felipensp felipensp marked this pull request as ready for review March 15, 2024 13:58
Comment on lines 1258 to 1260
allowed_ins := ['add', 'adc', 'and', 'btc', 'btr', 'bts', 'cmpxchg', 'cmpxch8b',
'cmpxchg16b', 'dec', 'inc', 'neg', 'not', 'or', 'sbb', 'sub', 'xor', 'xadd',
'xchg']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a const instead? I think this may be used in future as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would be a bit nicer as a constant, for code that uses lots of inline assembly, it will avoid some allocations. On the other hand, such code is rare, while a constant will be initialized every time the compiler is ran 🤔 .

Still, I think that the benefit of not doing allocations each time is greater.

Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work.

@spytheman spytheman merged commit 4221522 into vlang:master Mar 16, 2024
54 checks passed
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.

None yet

3 participants