Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

VM intermediate representation #984

Merged
merged 10 commits into from May 13, 2019
Merged

Conversation

troelsfr
Copy link
Contributor

No description provided.

@troelsfr troelsfr requested review from a team May 12, 2019 20:29
Copy link
Member

@ejfitzgerald ejfitzgerald left a comment

Choose a reason for hiding this comment

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

Makes some level of sense to me. I have a couple of questions more for my on curiosity. Difficult to review 100% when such a large code base. Since this blocks a number of things I suggest that we simply get it merged in and deal with the fallout

lhs->matrix.InlineSubtract(rhs->matrix);
Ptr<Matrix> m = AcquireMatrix(vm_, type_id_, element_type_id_, lhs_rows, rhs_columns);
// TODO(tfr): use blas
TODO_FAIL("Use BLAS TODO");
Copy link
Member

Choose a reason for hiding this comment

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

Not really sure if we want TODO_FAILS right in the middle here. Does it just make sense to set a RuntimeError on the VM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, agreed - we should use runtime error in the VM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change this tonight, but it might be worthwhile to wait for @baykaner to land the blas functionality and just fix this properly.

static TypeId const Float64 = 13;
static TypeId const PrimitiveMaxId = 13;
static TypeId const String = 14;
static TypeId const Address = 15;
Copy link
Member

Choose a reason for hiding this comment

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

Out of interest why does the Address get a static TypeId and State does not? Or am I missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect that it is because these are the types which has builtin literals. As such, Address should not be there in the first place since it does not have a literal. Rather, it should have been added as a module extension (unless there is a good reason why this was brought into the core of the VM).


IR &IR::operator=(IR &&other)
{
if (&other != this)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure you need to check this in a move assignment. Also, I'd consider making this operator noexcept.

@troelsfr troelsfr merged commit 9cbff4b into fetchai:develop May 13, 2019
@robertdickson robertdickson deleted the vm_ir_merge branch May 22, 2019 13:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants