Skip to content

Added Vector support for primitive methods. #68

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

Open
wants to merge 67 commits into
base: master
Choose a base branch
from

Conversation

rhys-h-walker
Copy link

@rhys-h-walker rhys-h-walker commented May 30, 2025

This PR implements the standard Vector class as well as the Vector from the Are We Fast Yet benchmarks as primitives. The Vector object is represented by VMVector for convenience, and most of Vector's methods are lowered to primitives.

Limitations

Because SOM++ is implemented as a non-recursive interpreter, primitives cannot make message sends that need to return execution to the primitive.
This means, we cannot lower any method that needs to activate a block passed as a parameter, and we can't implement #contains: and #indexOf:, because they rely on the #= message to determine equality.

Comment on lines 114 to 177
vm_oop_t VMVector::RemoveObj(vm_oop_t other) {
const int64_t first = INT_VAL(load_ptr(this->first));
const int64_t last = INT_VAL(load_ptr(this->last));
VMArray* storage = load_ptr(this->storage);

for (int64_t i = first - 1; i < last - 1; ++i) {
vm_oop_t current = storage->GetIndexableField(i);

// Check where integers are tagged or references can be checked
if (current == other) {
Remove(NEW_INT(i - first + 2)); // Convert to 1-indexing
return load_ptr(trueObject);
}
}
return load_ptr(falseObject);
}

vm_oop_t VMVector::Remove(vm_oop_t inx) {
const int64_t first = INT_VAL(load_ptr(this->first));
int64_t last = INT_VAL(load_ptr(this->last));
VMArray* storage = load_ptr(this->storage);
int64_t index = INT_VAL(inx);

if ((last - first) != 0 && index == 0) {
index = index;
}

if (index < 1 || index > last - first) {
IndexOutOfBounds();
}
Copy link
Member

Choose a reason for hiding this comment

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

This code here is a bit odd. I think Remove(inx) is not used anywhere else, and allocating an int here seems unnecessary for a helper.
But the code is also too general, I think.
If this is truly only a helper for RemoveObj() (if I didn't miss anything), then it doesn't need to handle the failure case, i.e., IndexOutOfBounds

@smarr smarr force-pushed the master branch 3 times, most recently from 760116b to e979852 Compare June 18, 2025 20:23
Copy link
Member

@smarr smarr left a comment

Choose a reason for hiding this comment

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

Some quick comments. Thanks!

rhys-h-walker and others added 21 commits July 8, 2025 15:10
- making the fields explicit makes usage easier
- also need to use NEW_INT and INT_VAL macros when accessing integer values, because we use integer tagging, and in most cases, these integers are not proper objects
- remove the static accessor methods in favor of direct field accesses
- change constructor to take explicit arguments for first, last, and storage
  - this simplifies the initialization of the VMVector object
  - in Universe, remove the allocation of large objects in the old generation, since all VMVector objects are small, just having three fields. The storage array may be old when it is large though

Signed-off-by: Stefan Marr <git@stefan-marr.de>
- initialize vectorClass correctly
- fixes for the bounds checking
- #at:put:
- #append:
- #first
- #last
…d in the Vector.som file, now only in Vector.cpp
- #removeFirst
- #contains:
- #indexOf:
- fixed indexable field bounds checking
- #remove:
- #asArray
- #new
…ining purposes, changed name of contains to Contains for consistency
This removes #contains: and #indexOf: because they require a message send of #= to work, which means we can’t support it in the current SOM++ design.
When sending the message, we yield back to the bytecode loop, which is not recursive, and thus, can’t come back to the primitive code.
- log when the vector primitives are active
…lib/Vector and load the correct primitive methods for each imeplementation
rhys-h-walker and others added 24 commits July 8, 2025 15:11
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
- doesn’t need TestWithParsing
- should use #include <> for standard library

Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
- fix Main.cpp

Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
The tests can use CPPUNIT_ASSERT_EQUAL and we can reduce the number of reinterpret_casts.
The helper method is also not needed, it just obscures what is tested.

Signed-off-by: Stefan Marr <git@stefan-marr.de>
… and variables

This is mostly to match the existing code, and avoid introducing a new style.

Signed-off-by: Stefan Marr <git@stefan-marr.de>
@smarr
Copy link
Member

smarr commented Jul 8, 2025

@rhys-h-walker I just rebased this PR, and did a bit of work on the flag you added.
I had already forgotten, that we already had the printing of mismatching hashes.
So, I thought, it would instead be useful to make the flag check that there are no mismatches.
I am using this now in CI to see whether they are up to date.
Would be great if you could look at the last two commits and check whether you see any issues.

Thanks!

This allows us to use the flag in CI and check that the hashes are correct.

Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
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.

2 participants