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

Implement POPCNT/LZCNT/TZCNT #131

Merged
merged 2 commits into from Jan 29, 2020
Merged

Conversation

soc
Copy link
Contributor

@soc soc commented Jan 7, 2020

Implements the three examples from #117.

@soc

This comment has been minimized.

@dinfuehr
Copy link
Owner

dinfuehr commented Jan 8, 2020

Already looks really good 👍 There can't miss much, I can help you out here - although probably not before the weekend.

@soc

This comment has been minimized.

@soc soc force-pushed the topic/bitops branch 5 times, most recently from bc1a70c to a922254 Compare January 10, 2020 04:17
@soc

This comment has been minimized.

@soc soc force-pushed the topic/bitops branch 7 times, most recently from 6ac9206 to c5882c3 Compare January 13, 2020 16:18
@soc soc marked this pull request as ready for review January 13, 2020 16:19
dora/src/masm/x64.rs Outdated Show resolved Hide resolved
@soc

This comment has been minimized.

@dinfuehr
Copy link
Owner

I will try to take a look the next few days. Can you please rebase it to master?

I suppose there is something wrong with the encoding. Did you try --emit-asm to see whether it prints the instruction you intend?

@soc

This comment has been minimized.

@soc soc force-pushed the topic/bitops branch 2 times, most recently from 6581384 to fa4bc4c Compare January 14, 2020 01:19
@soc

This comment has been minimized.

@soc

This comment has been minimized.

@dinfuehr
Copy link
Owner

Works on my Macbook. tester.rb doesn't support this right now, but you can just add a puts statement into the run_test method to print stdout/stderr.

I fear that we might have to introduce some form of check and fallback for this...

@@ -439,7 +456,7 @@ fn internal_method<'ast>(vm: &mut VM<'ast>, clsid: ClassId, name: &str, kind: Fc
let mtd = vm.fcts.idx(mid);
let mut mtd = mtd.write();

if mtd.name == name && mtd.internal {
if mtd.name == name {
mtd.kind = kind;
mtd.internal_resolved = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we set mtd.internal = true here too?

Copy link
Owner

Choose a reason for hiding this comment

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

It is true if the function is marked @internal, so I wouldn't set it here. I am wondering about replacing kind here though: This means that the source of the function is never typechecked if the instruction is supported. You could write anything parsable in there and Dora will happily run without failures as long as the assembly instruction is available. I think we should store the intrinsic not in kind but in a separate new field, e.g. intrinsic: Option<Intrinsic> in vm::Fct. This ensures that the function is always typechecked. In baseline::codegen::get_intrinsic we first check kind of the function and if that is not an intrinsic already also the intrinsic field.

This function internal_method wouldn't need to change at all, we would add another function e.g. conditional_intrinsic_method which sets intrinsic instead of kind and is used for optional/conditional intrinsics.

Copy link
Contributor Author

@soc soc Jan 26, 2020

Choose a reason for hiding this comment

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

Wouldn't it be cleaner to run typeck::check before we are adding the internal methods?

Agreed on optional intrinsic – but maybe it makes sense to have both the source code fct and the intrinsic marker as optional fields, such that all combinations can be represented faithfully?

EDIT: I moved prelude::internal_functions(vm) after typeck::check and verified that

  • the methods get typechecked in every case
  • the right implementation gets chosen

Copy link
Owner

Choose a reason for hiding this comment

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

I am still not convinced that replacing kind is the way to go - but since I can't name an example where we need it in the runtime, we can do this.

@soc soc force-pushed the topic/bitops branch 3 times, most recently from 94f82d2 to 576eefb Compare January 22, 2020 23:41
Copy link
Owner

@dinfuehr dinfuehr left a comment

Choose a reason for hiding this comment

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

Great work! Pretty cool that with this PR we also have some infrastructure to query cpu support. I have proposed a few minor changes.

dora/Cargo.toml Outdated Show resolved Hide resolved
dora/src/semck/prelude.rs Outdated Show resolved Hide resolved
dora/src/semck/prelude.rs Outdated Show resolved Hide resolved
dora/src/semck/prelude.rs Outdated Show resolved Hide resolved
@@ -439,7 +456,7 @@ fn internal_method<'ast>(vm: &mut VM<'ast>, clsid: ClassId, name: &str, kind: Fc
let mtd = vm.fcts.idx(mid);
let mut mtd = mtd.write();

if mtd.name == name && mtd.internal {
if mtd.name == name {
mtd.kind = kind;
mtd.internal_resolved = true;
Copy link
Owner

Choose a reason for hiding this comment

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

It is true if the function is marked @internal, so I wouldn't set it here. I am wondering about replacing kind here though: This means that the source of the function is never typechecked if the instruction is supported. You could write anything parsable in there and Dora will happily run without failures as long as the assembly instruction is available. I think we should store the intrinsic not in kind but in a separate new field, e.g. intrinsic: Option<Intrinsic> in vm::Fct. This ensures that the function is always typechecked. In baseline::codegen::get_intrinsic we first check kind of the function and if that is not an intrinsic already also the intrinsic field.

This function internal_method wouldn't need to change at all, we would add another function e.g. conditional_intrinsic_method which sets intrinsic instead of kind and is used for optional/conditional intrinsics.

@soc soc force-pushed the topic/bitops branch 4 times, most recently from 8d03dec to c080fad Compare January 26, 2020 23:01
@soc
Copy link
Contributor Author

soc commented Jan 26, 2020

@dinfuehr Do you think this is good to go as it is now?

@@ -42,6 +42,43 @@
@internal fun unaryMinus() -> Int;
@internal fun not() -> Int;

@internal fun countZeroBits() -> Int = self.not().countOneBits();
@internal fun countOneBits() -> Int {
Copy link
Contributor Author

@soc soc Jan 26, 2020

Choose a reason for hiding this comment

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

The change in internalck now allows marking methods and functions with implementations as internal.

I think this is good because it makes it easier to see where some intrinsic could come into play.

@@ -155,7 +156,7 @@ fn internalck<'ast>(vm: &VM<'ast>) {
let method = vm.fcts.idx(*method);
let method = method.read();

if method.internal && !method.internal_resolved {
if method.internal && !method.internal_resolved && method.kind.is_definition() {
Copy link
Contributor Author

@soc soc Jan 26, 2020

Choose a reason for hiding this comment

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

This change means that having @internal, but not having decided to use an intrinsic is now only considered to be a problem if we don't have an implementation in the source code.

@soc
Copy link
Contributor Author

soc commented Jan 28, 2020

@dinfuehr is this good to go?

dora/src/semck.rs Outdated Show resolved Hide resolved
@dinfuehr dinfuehr merged commit e24f173 into dinfuehr:master Jan 29, 2020
@dinfuehr
Copy link
Owner

Thanks!

@soc
Copy link
Contributor Author

soc commented Jan 29, 2020

Thank you too!

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

2 participants