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

Fix - JIT ExceededMaxInstructions and ExecutionOverrun #591

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

Lichtso
Copy link

@Lichtso Lichtso commented Sep 12, 2024

Thanks to @ravyu-jump, @ripatel-fd and @topointon-jump for finding this.

@Lichtso Lichtso merged commit 8747f87 into main Sep 12, 2024
12 checks passed
@Lichtso Lichtso deleted the fix/jit_execution_overrun_instruction_meter branch September 12, 2024 14:04
@Lichtso Lichtso changed the title Fix - JIT instruction meter in ExecutionOverrun Fix - JIT ExceededMaxInstructions and ExecutionOverrun Sep 12, 2024
@Lichtso Lichtso requested a review from LucasSte September 25, 2024 07:57
);
test_interpreter_and_jit_asm!(
"
add r1, 0",

Choose a reason for hiding this comment

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

Why does this exceed maximum instructions while the lddw case below does not? Add is one instruction while lddw is two.

Choose a reason for hiding this comment

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

Ah, Lddw accounts for a single instruction, not two. Let me reformulate my question. Why a does single instruction raise the ExceededMaxInstructions when we only request one unit for execution?

Copy link
Author

Choose a reason for hiding this comment

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

lddw only costs one CU. And this tests add, not lddw here.

Choose a reason for hiding this comment

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

lddw only costs one CU.

I wrote exactly that in my previous comment:

Ah, Lddw accounts for a single instruction, not two

There is no need to repeat what I said. You've probably missed my comment. Now, my question is:

Why a does single instruction raise the ExceededMaxInstructions when we only request one unit for execution?

So in the add test, we only have a single add instruction and request one CU, but we raise an ExceededMaxInstructions error. Why is this the case?

And this tests add, not lddw here.

I didn't understand how this answers my question.

Choose a reason for hiding this comment

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

So in the add test, we only have a single add instruction and request one CU, but we raise an ExceededMaxInstructions error. Why is this the case?

I suppose logically, the ExceededMaxInstructions and ExecutionOverrun faults happen after the same time here, and it's not clear what to return.
After add is executed, the virtual CPU is in a state where it expects another instruction. But at this point CU==0 and PC is OOB, so the next instruction would fault for either of these reasons.

I suppose you could specify that instruction fetch occurs before CU checks, but that's just going to inhibit future possible optimizations.

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.

3 participants