[wasm][shared] Fix write barrier DCHECK for unshared cmpxchg of tagged [v8/v8 : main]
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
PTAL. I still suspect that the `test262` failiures in `v8_linux_rel` are infra-issues given that the bot says `=== All tests succeeded` in the failing step.
Related details
- Jakob Kummerow
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
3 comments
LGTM with minor comments. Thanks for fixing this!
// Reuse eax (the result register) as a scratch register for the write
// barrier.This comment was moved up, you can drop it here.
And as a minor optimization, I think the next two lines could make use of the new `done` label if you reorder them:
```
__ JumpIfSmi(written_value, &done);
__ Push(eax);
```
Could even consider writing `Push(scratch)` and `Pop(scratch)`; it's a bit unfortunate that the `scratch = eax` definition has to move further away now.
if (i % 10 === 0) gc();Doing 100 full GC runs is perhaps a bit slow; any chance to speed this test up?
Related details
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Commit-Queue | +1 |
2 comments
// Reuse eax (the result register) as a scratch register for the write
// barrier.This comment was moved up, you can drop it here.
And as a minor optimization, I think the next two lines could make use of the new `done` label if you reorder them:
```
__ JumpIfSmi(written_value, &done);
__ Push(eax);
```
Could even consider writing `Push(scratch)` and `Pop(scratch)`; it's a bit unfortunate that the `scratch = eax` definition has to move further away now.
If you'd like to optimize, I guess we'd want to remove the `done` label and instead only have one label? (PTAL if you agree, otherwise I can reintroduce a second label.)
Doing 100 full GC runs is perhaps a bit slow; any chance to speed this test up?
My bad, seems like one gc might be enough, let's do 2. I verified that it reproduces on `origin/main` in that case.
Related details
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
1 comment
// Reuse eax (the result register) as a scratch register for the write
// barrier.Matthias LiedtkeThis comment was moved up, you can drop it here.
And as a minor optimization, I think the next two lines could make use of the new `done` label if you reorder them:
```
__ JumpIfSmi(written_value, &done);
__ Push(eax);
```
Could even consider writing `Push(scratch)` and `Pop(scratch)`; it's a bit unfortunate that the `scratch = eax` definition has to move further away now.
If you'd like to optimize, I guess we'd want to remove the `done` label and instead only have one label? (PTAL if you agree, otherwise I can reintroduce a second label.)
Would be nice to get by with one label, but I don't think that works: now coming back from the OOL code would jump past the `Pop(scratch)` instruction.
Related details
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |