[XL] Change in dart/sdk[main]: [vm] Add graph intrinsics for ThreadLocal hasValue and getValue methods.
Alexander Aprelev (Gerrit)
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
this is what I had in mind regarding graph intrinsics implementation for ThreadLocal getValue/hasValue.
Related details
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 3 comments![]( "Open in Gerrit")
V(Thread, _, thread_locals, false, FINAL)Supposedly this is a list of _untagged_ fields (e.g. fields which contain some raw pointers - rather than pointers to Dart objects). But `thread_locals` is a tagged pointer.
Also 4th parameter is `gc_may_move` and it can't be `false` because GC certainly can move `thread_locals`.
builder->AddInstruction(new CheckClassInstr(new Value(array), DeoptId::kNone,`CheckClassInstr` implies some sort of speculative optimization. It is not speculative here - we know it is `GrowableArray`, this should be encoded in the type information on the `Slot::Thread_thread_locals()`.
bool had_value = variable.hasValue;nit: names in Dart should be `camelCase` not `snake_case`.
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
4 comments
Supposedly this is a list of _untagged_ fields (e.g. fields which contain some raw pointers - rather than pointers to Dart objects). But `thread_locals` is a tagged pointer.
Also 4th parameter is `gc_may_move` and it can't be `false` because GC certainly can move `thread_locals`.
Done
builder->AddInstruction(new CheckClassInstr(new Value(array), DeoptId::kNone,`CheckClassInstr` implies some sort of speculative optimization. It is not speculative here - we know it is `GrowableArray`, this should be encoded in the type information on the `Slot::Thread_thread_locals()`.
Done
nit: names in Dart should be `camelCase` not `snake_case`.
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 2 comments![]( "Open in Gerrit")
Are failing tests caused by this change?
Definition* result = builder.AddDefinition(new StrictCompareInstr(Normally `LoadIndexedInstr::ComputeType(...)` would return a type which says that the value can't be sentinel. (See `ComputeArrayElementType` in `type_propagator.cc`). This means there is a high chance that this sort of comparison will be optimized away.
You need to fix the type of `LoadIndexedInstr` to prevent this from happening.
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
Are failing tests caused by this change?
Yeah, CheckClassInstr that I removed in latest patchset used to catch and send to runtime cases when thread_locals array is null, was not initialized yet.
Related details
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
3 comments
Alexander AprelevAre failing tests caused by this change?
Yeah, CheckClassInstr that I removed in latest patchset used to catch and send to runtime cases when thread_locals array is null, was not initialized yet.
Done
builder->AddInstruction(new CheckClassInstr(new Value(array), DeoptId::kNone,Alexander Aprelev`CheckClassInstr` implies some sort of speculative optimization. It is not speculative here - we know it is `GrowableArray`, this should be encoded in the type information on the `Slot::Thread_thread_locals()`.
Done
It can be null as well, in which case we do want to fall back to runtime implementation.
Definition* result = builder.AddDefinition(new StrictCompareInstr(Normally `LoadIndexedInstr::ComputeType(...)` would return a type which says that the value can't be sentinel. (See `ComputeArrayElementType` in `type_propagator.cc`). This means there is a high chance that this sort of comparison will be optimized away.
You need to fix the type of `LoadIndexedInstr` to prevent this from happening.
Done
Related details
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 1 comment![]( "Open in Gerrit")
Definition* array = builder->AddDefinition(On the deeper thought: I am not sure why `thread_locals` are even represented as a growable array. it's Length is always set equal to Capacity.
So I suggest representing it as a non-nullable Array instead, then you don't need any of this. You can initialize it to Object::empty_array which becomes available after VM isolate bootstrap is complete.
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
1 comment
On the deeper thought: I am not sure why `thread_locals` are even represented as a growable array. it's Length is always set equal to Capacity.
So I suggest representing it as a non-nullable Array instead, then you don't need any of this. You can initialize it to Object::empty_array which becomes available after VM isolate bootstrap is complete.
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |