-
Notifications
You must be signed in to change notification settings - Fork 122
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
FEXQConfig: Add strict split-lock option #4025
FEXQConfig: Add strict split-lock option #4025
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good minus my remarks on the UI layout, but do we anticipate this being used often enough to expose it in the main tabs rather than solely under Advanced?
Source/Tools/FEXQonfig/main.qml
Outdated
ConfigCheckBox { | ||
leftPadding: 24 | ||
text: qsTr("Strict in-process split-lock atomics") | ||
tooltip: qsTr("Controls split-lock emulation strictness") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tooltip doesn't add much. Either it should meaningfully elaborate on the feature, or it should be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the tooltip that it controls if split-locks can tear or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it still need a tooltip if the whole text is changed to "Enable non-tearing split-lock atomics"? Not sure how critical it is to mention them being in-process, since there's no other option that it could be confused with anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, removed the tooltip and changed the option name
Source/Tools/FEXQonfig/main.qml
Outdated
@@ -603,6 +603,12 @@ ApplicationWindow { | |||
tooltip: qsTr("Controls TSO emulation on memcpy/memset instructions") | |||
config: "MemcpySetTSOEnabled" | |||
} | |||
ConfigCheckBox { | |||
leftPadding: 24 | |||
text: qsTr("Strict in-process split-lock atomics") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this intentionally positioned in the middle of the three other options? It looks rather awkward in practice. It should probably be below the other three options.
Also, could you confirm this is only used for normal TSO operation and that it is ignored for paranoid TSO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it is independent of the these TSO toggles. Moved it below the three radio buttons since it is still part of the memory model emulation.
bb35d11
to
c68a7bf
Compare
I'm already using this a lot for testing purposes, very useful to look at the telemetry to inform if split-locks should be strict and that a crash should be verified with this option toggled. |
This was missed in the initial FEXQConfig implementation since it was implemented at the same time.
c68a7bf
to
ae545b8
Compare
This was missed in the initial FEXQConfig implementation since it was implemented at the same time.