[turboshaft] Add verification pass for load/store MemoryRepresentation [v8/v8 : main]
Darius Mercadier (Gerrit)
Darius Mercadier added 2 comments![]( "Open in Gerrit")
PTAL! (feel free to take your time: I'm out next week, and I don't plan on landing before I'm back)
DEFINE_BOOL(turboshaft_load_store_verification, true,TODO: make experimental before landing.
Related details
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Victor Gomes added 5 comments![]( "Open in Gerrit")
#ifdef DEBUGNit: Not sure if it is possible, but one could avoid the #ifdefs here if we define an empty reducer for non-debug builds, since we already do a #ifdef around the definition of the class anyways. The compiler should be able to ignore it...
is_wrong = __ Word32Equal(__ IsSmi(V<Object>::Cast(value)), 0);I guess TS doesn't have a not operation ?
if (expected_repr != any_of(MemoryRepresentation::TaggedSigned(),
MemoryRepresentation::TaggedPointer())) {
return;
}Why can we not check for other representations?
#if V8_COMPRESS_POINTERS
__ Word32Constant(0), MemoryRepresentation::Uint32(),
#else
__ WordPtrConstant(0), MemoryRepresentation::UintPtr(),
#endifNit: I guess a follow up: we could have a CompressPtr type. So this would be
`__ CompressPtrConstant(0), MemoryRepresentation::CompressPtr()` .
Related details
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Victor Gomes added 1 comment![]( "Open in Gerrit")
if (expected_repr != any_of(MemoryRepresentation::TaggedSigned(),
MemoryRepresentation::TaggedPointer())) {
return;
}Why can we not check for other representations?
Edit: Right, you are just checking sminess...
Related details
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 5 comments![]( "Open in Gerrit")
Thanks for the comments, Victor :)
Nit: Not sure if it is possible, but one could avoid the #ifdefs here if we define an empty reducer for non-debug builds, since we already do a #ifdef around the definition of the class anyways. The compiler should be able to ignore it...
That's indeed possible: nothing prevents us from defining an empty reducer and it will just be ignored by the reductions. It will still show up in the reducer stack (and stack traces if something goes wrong).
I personally would prefer not doing that though, because we then have this reducer that shows up in the stack trace / reducer stack, and it would be confusing to realize that it's actually empty.
is_wrong = __ Word32Equal(__ IsSmi(V<Object>::Cast(value)), 0);I guess TS doesn't have a not operation ?
We have a Word32BitwiseNot helper (which lowers to BitwiseXor(-1)), but this isn't what we want since it will turn both 1 and 0 into non-0. We'd need something to just flip the lowermost bit, and we don't have any operation that does this.
if (expected_repr != any_of(MemoryRepresentation::TaggedSigned(),
MemoryRepresentation::TaggedPointer())) {
return;
}Why can we not check for other representations?
For Loads, other representations dictate what we load, so there is nothing to be verified (ie, if the MemRep is Float64, then we'll emit a Float64 load so of course what we load is going to be a Float64). Unlike TaggedSigned/TaggedPointer where we just do the same Tagged load in both cases.
For Stores, we have verification when building an operation that the inputs have the right representation (this is done by ValidateOpInputRep). So if we have a Store with a Float64 rep and we pass it a Word32, then this other verification will already fail. But once again, this verification doesn't know the difference between TaggedSigned and TaggedPointer.
#if V8_COMPRESS_POINTERS
__ Word32Constant(0), MemoryRepresentation::Uint32(),
#else
__ WordPtrConstant(0), MemoryRepresentation::UintPtr(),
#endifNit: I guess a follow up: we could have a CompressPtr type. So this would be
`__ CompressPtrConstant(0), MemoryRepresentation::CompressPtr()` .
Maybe, but I'm not a huge fan of having a CompressPtr thingy existing on non-compress-ptr builds... But I do agree that this #if/#else isn't pretty, so what you suggest might still be better.. I'll think about it 😊
Related details
- Nico Hartmann
- Victor Gomes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Victor Gomes added 1 comment![]( "Open in Gerrit")
#if V8_COMPRESS_POINTERS
__ Word32Constant(0), MemoryRepresentation::Uint32(),
#else
__ WordPtrConstant(0), MemoryRepresentation::UintPtr(),
#endifDarius MercadierNit: I guess a follow up: we could have a CompressPtr type. So this would be
`__ CompressPtrConstant(0), MemoryRepresentation::CompressPtr()` .
Maybe, but I'm not a huge fan of having a CompressPtr thingy existing on non-compress-ptr builds... But I do agree that this #if/#else isn't pretty, so what you suggest might still be better.. I'll think about it 😊
Maybe it should be called `HeapPtr` then? Aka, a pointer to the heap that is compress in compress builds and uncompressed otherwise...
Related details
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
Daniel and Thibaud, any ideas about the red Windows bot that crashes in the register allocator? :/
Related details
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann added 1 comment![]( "Open in Gerrit")
Daniel and Thibaud, any ideas about the red Windows bot that crashes in the register allocator? :/
Unfortunately nothing comes to mind, but blame points to a CL in March 2025 https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/backend/register-allocator.cc;drc=f2058273f7e288855825ed6755bcf1d70224d25c;l=1503 with https://crrev.com/c/6336030, maybe Thibaud remembers more from then.
Related details
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
Daniel LehmannDaniel and Thibaud, any ideas about the red Windows bot that crashes in the register allocator? :/
Unfortunately nothing comes to mind, but blame points to a CL in March 2025 https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/backend/register-allocator.cc;drc=f2058273f7e288855825ed6755bcf1d70224d25c;l=1503 with https://crrev.com/c/6336030, maybe Thibaud remembers more from then.
I don't see a direct connection between the CL and the DCHECK at first glance so I'll have to take a closer look.
Until then a quick explanation of the DCHECK failure: it means that an instruction is using a non-allocatable register as a fixed input or output, which is generally not allowed. At least one place that relies on this assumption is spilling: regalloc only considers allocatable registers for spilling, so the corresponding virtual register would not be handled correctly.
It could be that there are specific case where this assumption can be relaxed. The CL that Daniel linked already relaxed it to allow temporaries to be non-allocatable for example.
Related details
- Daniel Lehmann
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
Daniel LehmannDaniel and Thibaud, any ideas about the red Windows bot that crashes in the register allocator? :/
Thibaud MichaudUnfortunately nothing comes to mind, but blame points to a CL in March 2025 https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/backend/register-allocator.cc;drc=f2058273f7e288855825ed6755bcf1d70224d25c;l=1503 with https://crrev.com/c/6336030, maybe Thibaud remembers more from then.
I don't see a direct connection between the CL and the DCHECK at first glance so I'll have to take a closer look.
Until then a quick explanation of the DCHECK failure: it means that an instruction is using a non-allocatable register as a fixed input or output, which is generally not allowed. At least one place that relies on this assumption is spilling: regalloc only considers allocatable registers for spilling, so the corresponding virtual register would not be handled correctly.
It could be that there are specific case where this assumption can be relaxed. The CL that Daniel linked already relaxed it to allow temporaries to be non-allocatable for example.
I was able to repro and investigate on a windows virtual machine.
The issue is with the RecordWriteSaveFP builtin. There is a call instruction there at pos 75, presumably introduced by this change, that takes an argument in rbx. The problem is that the builtin uses a restricted set of allocatable registers, defined here I think:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/codegen/x64/interface-descriptors-x64-inl.h;drc=2e71cd722cd12b2a1f9dfe30dfc09f6eea469c34;l=54
and indeed rbx is not in the list on Windows.
Darius Mercadier (Gerrit)
Darius Mercadier added 2 comments![]( "Open in Gerrit")
Daniel LehmannDaniel and Thibaud, any ideas about the red Windows bot that crashes in the register allocator? :/
Thibaud MichaudUnfortunately nothing comes to mind, but blame points to a CL in March 2025 https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/backend/register-allocator.cc;drc=f2058273f7e288855825ed6755bcf1d70224d25c;l=1503 with https://crrev.com/c/6336030, maybe Thibaud remembers more from then.
Thibaud MichaudI don't see a direct connection between the CL and the DCHECK at first glance so I'll have to take a closer look.
Until then a quick explanation of the DCHECK failure: it means that an instruction is using a non-allocatable register as a fixed input or output, which is generally not allowed. At least one place that relies on this assumption is spilling: regalloc only considers allocatable registers for spilling, so the corresponding virtual register would not be handled correctly.
It could be that there are specific case where this assumption can be relaxed. The CL that Daniel linked already relaxed it to allow temporaries to be non-allocatable for example.
I was able to repro and investigate on a windows virtual machine.
The issue is with the RecordWriteSaveFP builtin. There is a call instruction there at pos 75, presumably introduced by this change, that takes an argument in rbx. The problem is that the builtin uses a restricted set of allocatable registers, defined here I think:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/codegen/x64/interface-descriptors-x64-inl.h;drc=2e71cd722cd12b2a1f9dfe30dfc09f6eea469c34;l=54and indeed rbx is not in the list on Windows.
Great find, thanks a lot, Thibaud :)
I've fixed this by not doing the runtime call when compiling functions that aren't allowed to use all general purpose registers.
Related details
- Daniel Lehmann
- Nico Hartmann
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Hartmann (Gerrit)
Nico Hartmann voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nico Hartmann
| Code-Review | +1 |
2 comments
Pipeline(PipelineData* data, const Linkage* linkage) : data_(data) {I really don't like it that the linkage is an argument to the `Pipeline`. Can't we make this a field on the `PipelineData`? I am pretty sure this should already be there somewhere.
Edit: I see that currently we always pass in the `Linkage` to the functions on the `Pipeline`. Maybe we should change this also at some point.
If you feel like there is no easy alternative to putting this onto the pipeline, I'm okay with leaving it there for now, but please put a TODO at least. Thanks
Related details
- Daniel Lehmann
- Darius Mercadier
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Hartmann (Gerrit)
Nico Hartmann voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Daniel Lehmann
- Darius Mercadier
- Thibaud Michaud
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier voted and added 3 comments![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Commit-Queue | +2 |
3 comments
Pipeline(PipelineData* data, const Linkage* linkage) : data_(data) {I really don't like it that the linkage is an argument to the `Pipeline`. Can't we make this a field on the `PipelineData`? I am pretty sure this should already be there somewhere.
Edit: I see that currently we always pass in the `Linkage` to the functions on the `Pipeline`. Maybe we should change this also at some point.
If you feel like there is no easy alternative to putting this onto the pipeline, I'm okay with leaving it there for now, but please put a TODO at least. Thanks
So this CL adds the Linkage field to PipelineData, but the reason why it's set from Pipeline is to avoid forgetting setting it where it's needed. I didn't find a clean way to do this otherwise.
I was planning to remove all the passing around of Linkage in a follow up, now that it's easier to access through PipelineData. While I'm at it I'll try a bit harder to make Linkage an non-optional parameter of the PipelineData constructor :)
DEFINE_BOOL(turboshaft_load_store_verification, true,TODO: make experimental before landing.
- Daniel Lehmann
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Daniel Lehmann
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |