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.

Sign up for GitHub

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

Better C++ compatibility and fixes to conditional branch/switch handling #10

Merged
merged 7 commits into from
Nov 1, 2024
Merged

Better C++ compatibility and fixes to conditional branch/switch handling #10

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2677273
Add C++ header guard to extension header
mtehver Apr 18, 2022
a2a8534
Fix Phi node handling after executing OpBranchConditional, OpSwitch i…
mtehver Apr 18, 2022
fc2f6ad
A fix for Phi nodes by keeping track of current block immediate parents
mtehver Apr 19, 2022
463d733
Cleanup the code by keeping original OpLabel locations. Handling of b…
mtehver Apr 20, 2022
cb02416
Fixed OpAny and OpAll (previous version only checked first component …
mtehver Apr 21, 2022
1f653d0
Fixes to OpVectorExtractDynamic, OpVectorInsertDynamic, OpCompositeCo…
mtehver Jan 17, 2023
061b21a
Support for OpUndef, additional array bound-checking
mtehver Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
A fix for Phi nodes by keeping track of current block immediate parents
  • Loading branch information
@mtehver
mtehver committed Apr 19, 2022
commit fc2f6ad437ef14bb6eae4bdeec2b22d0ec1cc91b
1 change: 1 addition & 0 deletions inc/spvm/state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ typedef struct spvm_state {
spvm_word return_id;
spvm_word* function_stack_returns;
spvm_word* function_stack_cfg;
spvm_word* function_stack_cfg_parent;

void(*emit_vertex)(struct spvm_state*, spvm_word);
void(*end_primitive)(struct spvm_state*, spvm_word);
Expand Down
16 changes: 14 additions & 2 deletions src/opcode_execute.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1900,25 +1900,34 @@ void spvm_execute_OpPhi(spvm_word word_count, spvm_state_t state)

word_count = (word_count - 2) / 2;

int found = 0;
for (spvm_word i = 0; i < word_count; i++) {
spvm_word variable = SPVM_READ_WORD(state->code_current);
spvm_word parent = SPVM_READ_WORD(state->code_current);

if (parent == state->function_stack_cfg[state->function_stack_current]) {
if (parent == state->function_stack_cfg_parent[state->function_stack_current]) {
spvm_member_memcpy(state->results[id].members, state->results[variable].members, state->results[id].member_count);
found = 1;
break;
}
}
assert(found);
}
void spvm_execute_OpLabel(spvm_word word_count, spvm_state_t state)
{
spvm_word id = SPVM_READ_WORD(state->code_current);
state->function_stack_cfg[state->function_stack_current] = id;

// Make sure we do not overwrite parent block id if we encounter an explicit label after a jump
if (state->function_stack_cfg[state->function_stack_current] != id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we ever take this branch (except maybe for the entry point)? Couldn't we always handle the update of function_stack_cfg_parent, function_stack_cfg here instead of handling it in every jump instruction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it seems this can be done. When you debug current code you will see that branch target 'source locations' are not actual OpLabel instructions. This is due to the setup phase (spvm_setup_OpLabel) that moves source_location of its result record into the next instruction.

After changing the corresponding line in spvm_setup_OpLabel

state->results[id].source_location = state->code_current;

to

state->results[id].source_location = state->code_current - 2;

the code in actual branch instructions can be removed.

Should I update the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

The PR is valuable and LGTM as it is, no need to change it right away. IMO doing the processing in OpLabel (and changing source_location as you proposed) would simplify the code though as it removes duplication but we can also do this later on if you prefer. Thanks for looking into it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I already pushed this change, it really keeps the code cleaner.

state->function_stack_cfg_parent[state->function_stack_current] = state->function_stack_cfg[state->function_stack_current];
state->function_stack_cfg[state->function_stack_current] = id;
}
}
void spvm_execute_OpBranch(spvm_word word_count, spvm_state_t state)
{
spvm_word id = SPVM_READ_WORD(state->code_current);
state->code_current = state->results[id].source_location;
state->function_stack_cfg_parent[state->function_stack_current] = state->function_stack_cfg[state->function_stack_current];
state->function_stack_cfg[state->function_stack_current] = id;

state->did_jump = 1;
Expand All @@ -1929,6 +1938,7 @@ void spvm_execute_OpBranchConditional(spvm_word word_count, spvm_state_t state)
spvm_word true_branch = SPVM_READ_WORD(state->code_current);
spvm_word false_branch = SPVM_READ_WORD(state->code_current);

state->function_stack_cfg_parent[state->function_stack_current] = state->function_stack_cfg[state->function_stack_current];
if (state->results[cond].members[0].value.b) {
state->code_current = state->results[true_branch].source_location;
state->function_stack_cfg[state->function_stack_current] = true_branch;
Expand All @@ -1955,6 +1965,7 @@ void spvm_execute_OpSwitch(spvm_word word_count, spvm_state_t state)

if (val == lit) {
state->code_current = state->results[lbl].source_location;
state->function_stack_cfg_parent[state->function_stack_current] = state->function_stack_cfg[state->function_stack_current];
state->function_stack_cfg[state->function_stack_current] = lbl;
found = 1;
break;
Expand All @@ -1963,6 +1974,7 @@ void spvm_execute_OpSwitch(spvm_word word_count, spvm_state_t state)

if (!found) {
state->code_current = state->results[def_lbl].source_location;
state->function_stack_cfg_parent[state->function_stack_current] = state->function_stack_cfg[state->function_stack_current];
state->function_stack_cfg[state->function_stack_current] = def_lbl;
}

Expand Down
5 changes: 5 additions & 0 deletions src/state.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,14 @@ void spvm_state_prepare(spvm_state_t state, spvm_word fnLocation)
state->function_stack_info = (spvm_result_t*)malloc(state->function_stack_count * sizeof(spvm_result_t));
state->function_stack_returns = (spvm_word*)malloc(state->function_stack_count * sizeof(spvm_word));
state->function_stack_cfg = (spvm_word*)malloc(state->function_stack_count * sizeof(spvm_word));
state->function_stack_cfg_parent = (spvm_word*)malloc(state->function_stack_count * sizeof(spvm_word));
}

state->function_stack_current = 0;
state->function_stack[0] = state->code_current;
state->function_stack_info[0] = state->current_function;
state->function_stack_cfg[0] = 0;
state->function_stack_cfg_parent[0] = 0;
state->did_jump = 0;
state->discarded = 0;
state->instruction_count = 0;
Expand Down Expand Up @@ -417,12 +419,14 @@ void spvm_state_push_function_stack(spvm_state_t state, spvm_result_t func, spvm
state->function_stack_info = (spvm_result_t*)realloc(state->function_stack_info, state->function_stack_count * sizeof(spvm_result_t));
state->function_stack_returns = (spvm_word*)realloc(state->function_stack_returns, state->function_stack_count * sizeof(spvm_word));
state->function_stack_cfg = (spvm_word*)realloc(state->function_stack_cfg, state->function_stack_count * sizeof(spvm_word));
state->function_stack_cfg_parent = (spvm_word*)realloc(state->function_stack_cfg_parent, state->function_stack_count * sizeof(spvm_word));
}

state->function_stack[state->function_stack_current] = func->source_location;
state->function_stack_info[state->function_stack_current] = func;
state->function_stack_returns[state->function_stack_current] = func_res_id;
state->function_stack_cfg[state->function_stack_current] = 0;
state->function_stack_cfg_parent[state->function_stack_current] = 0;

state->code_current = func->source_location;
state->current_function = func;
Expand Down Expand Up @@ -460,6 +464,7 @@ void spvm_state_delete(spvm_state_t state)
free(state->function_stack_info);
free(state->function_stack_returns);
free(state->function_stack_cfg);
free(state->function_stack_cfg_parent);
}

free(state->results);
Expand Down