Store all permutations in a global shared array, so that one 32-bit index per register is sufficient. #239

mandolaerik · 2023-11-14T19:38:33Z

remove unused method
Avoid re-calculating read/write field order
Store field order as permutation indices instead of field references
Store all permutations in a global shared array, so that one 32-bit index per register is sufficient.

This saves some space

syssimics · 2023-11-14T19:45:09Z

Verification #12507032: fail

index per register is sufficient. Some sophisticated memo/startup trickery is used to clean up temporary data structures.

mandolaerik · 2023-11-14T19:59:51Z

This is yet unpolished, and meant as a basis for discussions (but at least 1.4/lib/fields passes).

does this approach make sense, or is it a dead end?
The memoization of _get_read_fields adds some ugliness -- just look at _cleanup_permutation_table_helper -- we can consider skipping this and keep hash tables instead, doing an identity_t lookup every time. Probably somewhat slower in runtime, and may use more or less mem, but the diff should not be dramatic.

syssimics · 2023-11-14T20:51:54Z

Verification #12507039: fail

lwaern-intel

I think this approach is... though not pretty, it is fine when adjusted. My big peeve is _permutation_table_helper(), but that's fixable (see comment.)

The guarantee we offer that the implicit calls of independent startups happen before independent startup memoized was to offer a degree of control when startup memoizeds had dependencies, so it's not like we're operating outside their boundary of what they were intended for (except _permutation_table_helper(), which, again, is fixable.)

lwaern-intel · 2023-11-15T07:06:57Z

include/simics/dmllib.h

+                int n = strlen(perm);
+                VGROW(h->table, n);
+                for (int i = 0; i < n; i++) {
+                        VSET(h->table, idx + i, perm[i] & 0x7f);


This bitmask is to be rid of the upper bit used to prevent the NUL char, yeah? Have a comment speaking to that effect. Also consider & ~(1 << 7), I prefer that pattern before literals.

lwaern-intel · 2023-11-15T07:31:14Z

lib/1.4/dml-builtins.dml

+    independent startup method _cleanup_permutation_table_helper() {
+        foreach reg in (each register in (dev)) {
+            reg._get_read_fields();
+            reg._get_write_fields();


These implicit discards hurt me and may cause Coverity to get mad. Consider binding these to local variables or make this PR dependent on #237 (hint hint nudge nudge review please)

lwaern-intel · 2023-11-15T07:44:26Z

lib/1.4/dml-builtins.dml

+                ++n;
+            }
+            _DML_add_read_permutation(h, cast(&reg, _traitref_t *)->id,
+                                      reg._sort_permutation(lsbs, n));


The result of _sort_permutation must be deallocated. Either by _permutation_table_helper() or by _DML_add_read/write_permutation.
Alterternatively, allocate the array on the stack, just like you do with lsbs.

lwaern-intel · 2023-11-15T07:45:58Z

include/simics/dmllib.h

+{
+        uint8 *ret = MM_MALLOC(VLEN(h->table), uint8);
+        memcpy(ret, VVEC(h->table), VLEN(h->table));
+        return ret;


Alternatively, detach the associated array and realloc it to free excess space. One way to do that without relying on vect.h internals would be:

VRESIZE_FREE(h->table, VLEN(h->table)); uint8 *ret = VVEC(h->table); VINIT(h->table);

lwaern-intel · 2023-11-15T07:56:09Z

lib/1.4/dml-builtins.dml

+        -> (const uint8 *) {
+        return _DML_field_permutations(_permutation_table_helper());
+    }
+    independent method _permutation_index_read(object o)


No compelling reason to have this and _permutation_index_write take object instead of register. The architecture is explicitly for field permutations, and its limitations means it cannot be reasonably used for anything else.

lwaern-intel · 2023-11-15T08:03:25Z

lib/1.4/dml-builtins.dml

+        }
+        return h;
+    }
+    independent startup memoized method _field_permutation_table()


If you want to be super-duper performance petty, you can consider the extern C approach for this too:

header %{ extern const uint8 * _field_permutation_array UNUSED; %} footer %{ const uint8 * _field_permutation_array UNUSED; %} extern const uint8 *_field_permutation_array;

... initialized in _cleanup_permutation_table_helper. This will eliminate the function call needed to retrieve the memoized value.

lwaern-intel · 2023-11-15T08:11:39Z

lib/1.4/dml-builtins.dml

-        local uint64 unmapped;
-        (num_fields, unmapped) = this._get_read_fields(fields);
+        local uint8 lsbs[64];
+        local uint8 msbs[64];


What's the benefit of leveraging these instead of doing:

local int f_lsb = fields[f].lsb; local int f_msb = f_lsb + fields[f].bitsize - 1;

in the .read_field() loop? Note you're not using them (even though you declare and initialize them) in write_register.

lwaern-intel · 2023-11-15T08:42:38Z

lib/1.4/dml-builtins.dml

    method post_init() default {}

    method destroy() default { }
+


Why device template instead of top-level?

lwaern-intel · 2023-11-15T08:58:05Z

lib/1.4/dml-builtins.dml

    method destroy() default { }
+
+    independent startup memoized method _permutation_table_helper()
+        -> (_permutation_helper_t *) {


Yeah I don't like this either, in particular I don't like the part that _cleanup_permutation_table_helper will invalidate the memoized value with that being impossible to check, so if something goes wrong and we have a usage past _cleanup_permutation_table_helper we'll segfault.
My suggestion is to leverage an externed C variable instead:

header %{ -- extern linkage to support --split-c-file extern _permutation_helper_t *_permutation_table_helper UNUSED; %} footer %{ _permutation_helper_t *_permutation_table_helper UNUSED; %} extern _permutation_helper_t *_permutation_table_helper; independent method _initialize_permutation_table_helper() { _permutation_table_helper = _DML_new_permutation_helper(); ... } independent startup method _cleanup_permutation_table_helper() { _initialize_permutation_table_helper(); ... _DML_destroy_permutation_table_helper(_permutation_table_helper): _permutation_table_helper = NULL: }

This way, we can guard all usages of _permutation_table_helper by assert _permutation_table_helper;.

lwaern-intel · 2023-11-15T10:48:18Z

We have segfaults, by the way! This may or may not be due to the thing I complained about, regardless, you'll want to track them down.

mandolaerik · 2023-11-24T22:28:21Z

This PR does demonstrate an interesting memoization technique, but after #243 it is no longer needed.

mandolaerik added 3 commits November 13, 2023 19:24

remove unused method

36f849b

Avoid re-calculating read/write field order

7323686

Store field order as permutation indices instead of field references

a29fbbd

This saves some space

mandolaerik requested a review from lwaern-intel November 14, 2023 19:41

Store all permutations in a global shared array, so that one 32-bit

5b52901

index per register is sufficient. Some sophisticated memo/startup trickery is used to clean up temporary data structures.

mandolaerik force-pushed the pr/store-all-permutations-in-a-global-shared-array-so-that-one-32-bit-index-per-register-is-sufficient branch from b97dc54 to 5b52901 Compare November 14, 2023 19:46

lwaern-intel requested changes Nov 15, 2023

View reviewed changes

This comment was marked as off-topic.

Sign in to view

mandolaerik requested a review from johanhogberg November 16, 2023 17:13

mandolaerik mentioned this pull request Nov 24, 2023

Use counting sort to sort in linear instead of quadratic time #243

Merged

mandolaerik closed this Nov 24, 2023

Store all permutations in a global shared array, so that one 32-bit index per register is sufficient. #239

Store all permutations in a global shared array, so that one 32-bit index per register is sufficient. #239

Uh oh!

Conversation

mandolaerik commented Nov 14, 2023

Uh oh!

syssimics commented Nov 14, 2023

Uh oh!

mandolaerik commented Nov 14, 2023

Uh oh!

syssimics commented Nov 14, 2023

Uh oh!

lwaern-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwaern-intel Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwaern-intel commented Nov 15, 2023

Uh oh!

This comment was marked as off-topic.

Uh oh!

mandolaerik commented Nov 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lwaern-intel Nov 15, 2023 •

edited

Loading