Skip to content

[vm] Add NoGC scopes to flow graph builder #48527

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
2 tasks
dcharkes opened this issue Mar 8, 2022 · 0 comments
Open
2 tasks

[vm] Add NoGC scopes to flow graph builder #48527

dcharkes opened this issue Mar 8, 2022 · 0 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Mar 8, 2022

We have at least two places (soon to be three) where we rely in IL that the GC doesn't run between certain IL instructions.

// No GC from here til LoadIndexed.
body += LoadUntagged(compiler::target::PointerBase::data_offset());
body += LoadLocal(arg_offset_not_null);
body += UnboxTruncate(kUnboxedFfiIntPtr);
body += LoadIndexed(typed_data_cid, /*index_scale=*/1,
/*index_unboxed=*/true, alignment);

// No GC from here til StoreIndexed.
body += LoadUntagged(compiler::target::PointerBase::data_offset());
body += LoadLocal(arg_offset_not_null);
body += UnboxTruncate(kUnboxedFfiIntPtr);
body += LoadLocal(arg_value_not_null);
if (kind == MethodRecognizer::kFfiStorePointer) {
// This can only be Pointer, so it is always safe to LoadUntagged.
body += LoadUntagged(compiler::target::PointerBase::data_offset());
body += ConvertUntaggedToUnboxed(kUnboxedFfiIntPtr);
} else {
// Avoid any unnecessary (and potentially deoptimizing) int
// conversions by using the representation consumed by StoreIndexed.
body += UnboxTruncate(
StoreIndexedInstr::RepresentationOfArrayElement(typed_data_cid));
if (kind == MethodRecognizer::kFfiStoreFloat ||
kind == MethodRecognizer::kFfiStoreFloatUnaligned) {
body += DoubleToFloat();
}
}
body += StoreIndexedTypedData(typed_data_cid, /*index_scale=*/1,
/*index_unboxed=*/true, alignment);

Currently, we:

  1. force optimize them
  2. prevent them from being inlined (by being force optimized)

Preventing inlining gives us enough confidence that code motion doesn't move any GCing instructions in between currently.

If we start inlining these, we need to take special care that code motion doesn't move an instruction that can trigger GC in between those instructions:

We should consider adding a NoGC-scope construct that:

  • Checks that only IL instructions with kNoGC are added to that scope
  • Prevents code motion of any kNoGC instructions out of that scope and prevents code motion of any non-kNoGC into that scope.

cc @mkustermann @mraleph (I don't believe we had a tracking issue yet.)

Edit: For the two existing cases running GC is fine, they happen to be idempotent, for the new case it is not. Having this NoGC-scope clarifies the difference.

@dcharkes dcharkes added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

1 participant