Skip to content

[vm] FlowGraphCompiler::EmitMove does not support kUnboxedFloat from const source to stack #53900

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

Closed
dcharkes opened this issue Oct 30, 2023 · 3 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. vm-technical-debt This label tries to capture all the technical debt that we have accumulated in the Dart VM

Comments

@dcharkes
Copy link
Contributor

For const moves, it uses the constant instruction:

ASSERT(source.IsConstant());
if (destination.IsFpuRegister() || destination.IsDoubleStackSlot() ||
destination.IsStackSlot()) {
Register tmp = allocator->AllocateTemporary();
source.constant_instruction()->EmitMoveToLocation(this, destination, tmp,
source.pair_index());
allocator->ReleaseTemporary();
} else {
source.constant_instruction()->EmitMoveToLocation(
this, destination, kNoRegister, source.pair_index());
}

And this instruction doesn't support kUnboxedFloat, and silently does the wrong thing on some archs:

Silent failure on arm32, assuming kTagged:

} else {
ASSERT(destination.IsStackSlot());
ASSERT(tmp != kNoRegister);
const intptr_t dest_offset = destination.ToStackSlotOffset();
if (RepresentationUtils::IsUnboxedInteger(representation())) {
int64_t v;
const bool ok = compiler::HasIntegerValue(value_, &v);
RELEASE_ASSERT(ok);
__ LoadImmediate(
tmp, pair_index == 0 ? Utils::Low32Bits(v) : Utils::High32Bits(v));
} else {
__ LoadObject(tmp, value_);
}
__ StoreToOffset(tmp, destination.base_reg(), dest_offset);
}

Discovered on https://dart-review.googlesource.com/c/sdk/+/284300/59

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. vm-technical-debt This label tries to capture all the technical debt that we have accumulated in the Dart VM labels Oct 30, 2023
@dcharkes dcharkes changed the title [vm] FlowGraphCompiler::EmitMove does not support kUnboxedFloat from const source [vm] FlowGraphCompiler::EmitMove does not support kUnboxedFloat from const source to stack Oct 30, 2023
@dcharkes
Copy link
Contributor Author

Our IL tests don't give us access to the assembler. Our assembler tests don't have a flow graph. There is actually no need for ConstantInstr::EmitMoveToLocation to take a FlowGraphCompiler as argument, it only uses the Assembler. However, then we'd lose the parallel with all EmitNativeCode methods. And it would subsequently make more sense to move the implementation into the assemblers instead of the il_xx.cc files.

Not sure if that refactoring would be worth it.

@dcharkes
Copy link
Contributor Author

So far the Assembler does not depend on Location. (And introducing that dependency currently creates an import cycle between the header files.)

The assembler is context agnostic, e.g. it doesn't know about IL, the FlowGraph, etc.
Location is mostly context agnostic. However, constant locations refer to a ConstantInstr which encapsulates the actual value and representation.
If we ignore the constant locations being backed by an instruction, then the target destination of a move is completely context agnostic, and we could have destination locations in the assembler.
(Thanks @sstrickl for some of these observations!)

@dcharkes
Copy link
Contributor Author

CL without refactoring: https://dart-review.googlesource.com/c/sdk/+/332761

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. vm-technical-debt This label tries to capture all the technical debt that we have accumulated in the Dart VM
Projects
None yet
Development

No branches or pull requests

1 participant