[sparkplug+] Support embedded feedback for Equal and Relational Comparison Operations [v8/v8 : main]
Wei, Yuheng (Gerrit)
Wei, Yuheng added 1 comment![]( "Open in Gerrit")
Related details
- Leszek Swirski
- Toon Verwaest
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Toon Verwaest (Gerrit)
Toon Verwaest voted and added 1 comment![]( "Open in Gerrit")
Votes added by Toon Verwaest
| Code-Review | +1 |
1 comment
bytecode array length: 34Not for this cl, but I was wondering if we could replace bitwise or with a table mapping from one combined feedback indicator to the next, based on observed inputs. E.g. combined_feedback = table[combined_feedback][new_feedback]. That way we could probably encode the embedded feedback into 1 byte instead of 2? I presume the perf diff is negligible.
Related details
- Leszek Swirski
- Wei, Yuheng
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wei, Yuheng (Gerrit)
Wei, Yuheng added 2 comments![]( "Open in Gerrit")
Hi Leszek, please also take a look :)
bytecode array length: 34Not for this cl, but I was wondering if we could replace bitwise or with a table mapping from one combined feedback indicator to the next, based on observed inputs. E.g. combined_feedback = table[combined_feedback][new_feedback]. That way we could probably encode the embedded feedback into 1 byte instead of 2? I presume the perf diff is negligible.
Shrinking the embedded feedback into 8 bits would avoid possible unaligned 16-bit r/w, and I think this will offer a slight perf benefit.
IIUC, although compare op feedback currently use 10 bits one-hot encoding ([link](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/common/globals.h;drc=8f35dec9947f2b0c21c18f68eba49154a26f6430;l=2444)), the **meaningful** states are not so much. We could use a 8 bits `state` to track states that are valuable to the optimizers, and leave those meaningless states (for example `kString | kSymbol`) in `kAny`. Then we can have feedback update like `new_state` = table[`old_state`][`input_type`].
Compare ops as an example, there are ~17 named feedback types. We would essentially need a 17x17 state transition map and a decoding table that are shared across sparkplug, maglev and tf.
Related details
- Leszek Swirski
- Toon Verwaest
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
- Toon Verwaest
- Wei, Yuheng
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Toon Verwaest (Gerrit)
Toon Verwaest added 1 comment![]( "Open in Gerrit")
bytecode array length: 34Wei, YuhengNot for this cl, but I was wondering if we could replace bitwise or with a table mapping from one combined feedback indicator to the next, based on observed inputs. E.g. combined_feedback = table[combined_feedback][new_feedback]. That way we could probably encode the embedded feedback into 1 byte instead of 2? I presume the perf diff is negligible.
Shrinking the embedded feedback into 8 bits would avoid possible unaligned 16-bit r/w, and I think this will offer a slight perf benefit.
IIUC, although compare op feedback currently use 10 bits one-hot encoding ([link](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/common/globals.h;drc=8f35dec9947f2b0c21c18f68eba49154a26f6430;l=2444)), the **meaningful** states are not so much. We could use a 8 bits `state` to track states that are valuable to the optimizers, and leave those meaningless states (for example `kString | kSymbol`) in `kAny`. Then we can have feedback update like `new_state` = table[`old_state`][`input_type`].
Compare ops as an example, there are ~17 named feedback types. We would essentially need a 17x17 state transition map and a decoding table that are shared across sparkplug, maglev and tf.
I we just have a transition map then the total number of states should be < 256 either way? I think it would be nice to avoid the cross-cacheline writes.
Related details
- Wei, Yuheng
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wei, Yuheng (Gerrit)
Wei, Yuheng voted Code-Review+1![]( "Open in Gerrit")
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wei, Yuheng (Gerrit)
Wei, Yuheng removed a vote from this change![]( "Open in Gerrit")
Removed Code-Review+1 by Wei, Yuheng <yuhen...@intel.com>
Related details
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wei, Yuheng (Gerrit)
Wei, Yuheng added 1 comment![]( "Open in Gerrit")
bytecode array length: 34Wei, YuhengNot for this cl, but I was wondering if we could replace bitwise or with a table mapping from one combined feedback indicator to the next, based on observed inputs. E.g. combined_feedback = table[combined_feedback][new_feedback]. That way we could probably encode the embedded feedback into 1 byte instead of 2? I presume the perf diff is negligible.
Toon VerwaestShrinking the embedded feedback into 8 bits would avoid possible unaligned 16-bit r/w, and I think this will offer a slight perf benefit.
IIUC, although compare op feedback currently use 10 bits one-hot encoding ([link](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/common/globals.h;drc=8f35dec9947f2b0c21c18f68eba49154a26f6430;l=2444)), the **meaningful** states are not so much. We could use a 8 bits `state` to track states that are valuable to the optimizers, and leave those meaningless states (for example `kString | kSymbol`) in `kAny`. Then we can have feedback update like `new_state` = table[`old_state`][`input_type`].
Compare ops as an example, there are ~17 named feedback types. We would essentially need a 17x17 state transition map and a decoding table that are shared across sparkplug, maglev and tf.
I we just have a transition map then the total number of states should be < 256 either way? I think it would be nice to avoid the cross-cacheline writes.
Yes. Since the total number of meaningful states is well under 256, 1 byte embedded feedback is enough. If this sounds good to you I can draft a follow-up CL later :)
Related details
- Toon Verwaest
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |