Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2391393003:
MIPS64: Fix Word32Compare turbofan operator implementation when comparing signed with unsigned oper… (Closed)
|
Created:
4 years, 2 months ago by ivica.bogosavljevic Modified:
4 years, 2 months ago CC:
v8-ports-mips_googlegroups.com, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionMIPS64: Fix Word32Compare turbofan operator implementation when comparing signed with unsigned operand
MIPS64 doesn't support Word32 compare instructions. Instead it relies
that the values in registers are correctly sign-extended and uses
Word64 comparison instead. This behavior is correct in most cases,
but doesn't work when comparing signed with unsigned operands.
The solution proposed here tries to match a comparison of signed
with unsigned operand, and perform Word32Compare simulation only
in those cases. Unfortunately, the solution is not complete because
it might skip cases where Word32 compare simulation is needed, so
basically it is a hack.
BUG=
TEST=mjsunit/compiler/uint32
Committed: https://crrev.com/7499d92d7f4074c3ad7461ea70cbf896e30e6866
Cr-Commit-Position: refs/heads/master@{#40398}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Simplify ToSimulateWord32Compare #
Total comments: 3
Patch Set 3 : Add runtime checks to make sure optimized compare always works #
Messages
Total messages: 34 (16 generated)
ivica.bogosavljevic@imgtec.com changed reviewers: + ahaas@chromium.org, bmeurer@chromium.org, miran.karic@imgtec.com, titzer@chromium.org
PTAL This is a workaround for implementation of Word32Compare for MIPS64. MIPS64 doesn't have a Word32Compare instructions, instead it relies that values in registers are properly sign-extended. This behavior is correct in most cases, but doesn't work when comparing signed with unsigned operands. If we were to implement Word32Compare simulation everywhere, we would need two additional instruction before each Word32Compare. In that case we get performance degradation of around 4% in Richard-octane test suite. I propose here a kind-a hack: we try to match unsigned with signed load and only there do Word32Compare simulation, otherwise we rely on Word64Compare since the operands are properly sign-extended. It is possible that this solution leaves some hidden bugs (although the bug causes failure in only one test, and with the fix there are no more failures). I need your comment: do you have idea of any other way to do this that is better than what is proposed here? Ivica
miran.karic@imgtec.com changed reviewers: + Miran.Karic@imgtec.com
Looks like a good solution to me. https://codereview.chromium.org/2391393003/diff/1/src/compiler/mips64/instruc... File src/compiler/mips64/instruction-selector-mips64.cc (right): https://codereview.chromium.org/2391393003/diff/1/src/compiler/mips64/instruc... src/compiler/mips64/instruction-selector-mips64.cc:1868: return (leftUnsigned && !rightUnsigned) || (!leftUnsigned && rightUnsigned); You can just have return IsNodeUnsigned(left) != IsNodeUnsigned(right);
ivica.bogosavljevic@imgtec.com changed reviewers: - Miran.Karic@imgtec.com
Reminder...
https://codereview.chromium.org/2391393003/diff/1/src/compiler/mips64/instruc... File src/compiler/mips64/instruction-selector-mips64.cc (right): https://codereview.chromium.org/2391393003/diff/1/src/compiler/mips64/instruc... src/compiler/mips64/instruction-selector-mips64.cc:1861: bool ToSimulateWord32Compare(Node* node) { I think you could even just inline this function and say "if(IsNodeUnsigned(node->InputAt(0)) != IsNodeUnsigned(node->InputAt(1)))"
PTAL
I think Ivica wants more answers to his first question (https://codereview.chromium.org/2391393003/#msg2) > I need your comment: do you have idea of any other way to do this that is better > than what is proposed here?
On 2016/10/13 at 08:00:24, Miran.Karic wrote: > I think Ivica wants more answers to his first question (https://codereview.chromium.org/2391393003/#msg2) > > I need your comment: do you have idea of any other way to do this that is better > > than what is proposed here? Are you asking whether there exists a better way to figure out if a TurboFan node returns an uint32 than you do in IsNodeUnsigned? I am not aware of a better way.
https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... File src/compiler/mips64/instruction-selector-mips64.cc (right): https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... src/compiler/mips64/instruction-selector-mips64.cc:1853: } else { I think you also have to deal with parameter and phi nodes here.
https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... File src/compiler/mips64/instruction-selector-mips64.cc (right): https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... src/compiler/mips64/instruction-selector-mips64.cc:1853: } else { On 2016/10/13 08:10:54, ahaas wrote: > I think you also have to deal with parameter and phi nodes here. Hi Ahaas! I looked at the code: the Phi and Select operators from common-operators.h don't store their representation using MachineType (that contains Unsigned/Signed information), instead they use MachineRepresentation (that contains only Word32/Word64).
On 2016/10/14 at 08:16:40, ivica.bogosavljevic wrote: > https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... > File src/compiler/mips64/instruction-selector-mips64.cc (right): > > https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... > src/compiler/mips64/instruction-selector-mips64.cc:1853: } else { > On 2016/10/13 08:10:54, ahaas wrote: > > I think you also have to deal with parameter and phi nodes here. > > Hi Ahaas! > > I looked at the code: the Phi and Select operators from common-operators.h don't store their representation using MachineType (that contains Unsigned/Signed information), instead they use MachineRepresentation (that contains only Word32/Word64). ah, in that case, never mind.
miran.karic@imgtec.com changed reviewers: + Miran.Karic@imgtec.com
lgtm
On 2016/10/14 08:17:30, ahaas wrote: > On 2016/10/14 at 08:16:40, ivica.bogosavljevic wrote: > > > https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... > > File src/compiler/mips64/instruction-selector-mips64.cc (right): > > > > > https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... > > src/compiler/mips64/instruction-selector-mips64.cc:1853: } else { > > On 2016/10/13 08:10:54, ahaas wrote: > > > I think you also have to deal with parameter and phi nodes here. > > > > Hi Ahaas! > > > > I looked at the code: the Phi and Select operators from common-operators.h > don't store their representation using MachineType (that contains > Unsigned/Signed information), instead they use MachineRepresentation (that > contains only Word32/Word64). > > ah, in that case, never mind. I was very reluctant whether to put it like this. Apparently there are some cases that we miss (e.g. Word32And that uses two unsigned will have result unsigned, if we compare it to Int32Constant we may get bad results). But on the other hand, of all 22000 tests in v8 suits there is one single test that fails, and the test itself tests UInt32ExternalArrays. So the solution here is not complete, but it is very good in terms of performance and coverage, so I think it's ok for it to land.
The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
titzer@chromium.org changed reviewers: - Miran.Karic@imgtec.com
https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... File src/compiler/mips64/instruction-selector-mips64.cc (right): https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... src/compiler/mips64/instruction-selector-mips64.cc:1853: } else { On 2016/10/14 08:16:40, ivica.bogosavljevic wrote: > On 2016/10/13 08:10:54, ahaas wrote: > > I think you also have to deal with parameter and phi nodes here. > > Hi Ahaas! > > I looked at the code: the Phi and Select operators from common-operators.h don't > store their representation using MachineType (that contains Unsigned/Signed > information), instead they use MachineRepresentation (that contains only > Word32/Word64). Is it possible to add some extra --debug-code checks to verify in debug mode that when this optimization occurs, it is correct? That will help flush out bugs.
On 2016/10/15 10:45:33, titzer wrote: > https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... > File src/compiler/mips64/instruction-selector-mips64.cc (right): > > https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... > src/compiler/mips64/instruction-selector-mips64.cc:1853: } else { > On 2016/10/14 08:16:40, ivica.bogosavljevic wrote: > > On 2016/10/13 08:10:54, ahaas wrote: > > > I think you also have to deal with parameter and phi nodes here. > > > > Hi Ahaas! > > > > I looked at the code: the Phi and Select operators from common-operators.h > don't > > store their representation using MachineType (that contains Unsigned/Signed > > information), instead they use MachineRepresentation (that contains only > > Word32/Word64). > > Is it possible to add some extra --debug-code checks to verify in debug mode > that when this optimization occurs, it is correct? That will help flush out > bugs. An excellent idea, thanks! I implemented it, have a look
On 2016/10/18 09:39:36, ivica.bogosavljevic wrote: > On 2016/10/15 10:45:33, titzer wrote: > > > https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... > > File src/compiler/mips64/instruction-selector-mips64.cc (right): > > > > > https://codereview.chromium.org/2391393003/diff/20001/src/compiler/mips64/ins... > > src/compiler/mips64/instruction-selector-mips64.cc:1853: } else { > > On 2016/10/14 08:16:40, ivica.bogosavljevic wrote: > > > On 2016/10/13 08:10:54, ahaas wrote: > > > > I think you also have to deal with parameter and phi nodes here. > > > > > > Hi Ahaas! > > > > > > I looked at the code: the Phi and Select operators from common-operators.h > > don't > > > store their representation using MachineType (that contains Unsigned/Signed > > > information), instead they use MachineRepresentation (that contains only > > > Word32/Word64). > > > > Is it possible to add some extra --debug-code checks to verify in debug mode > > that when this optimization occurs, it is correct? That will help flush out > > bugs. > > An excellent idea, thanks! I implemented it, have a look Thanks! LGTM
The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ivica.bogosavljevic@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from Miran.Karic@imgtec.com Link to the patchset: https://codereview.chromium.org/2391393003/#ps40001 (title: "Add runtime checks to make sure optimized compare always works")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MIPS64: Fix Word32Compare turbofan operator implementation when comparing signed with unsigned operand MIPS64 doesn't support Word32 compare instructions. Instead it relies that the values in registers are correctly sign-extended and uses Word64 comparison instead. This behavior is correct in most cases, but doesn't work when comparing signed with unsigned operands. The solution proposed here tries to match a comparison of signed with unsigned operand, and perform Word32Compare simulation only in those cases. Unfortunately, the solution is not complete because it might skip cases where Word32 compare simulation is needed, so basically it is a hack. BUG= TEST=mjsunit/compiler/uint32 ========== to ========== MIPS64: Fix Word32Compare turbofan operator implementation when comparing signed with unsigned operand MIPS64 doesn't support Word32 compare instructions. Instead it relies that the values in registers are correctly sign-extended and uses Word64 comparison instead. This behavior is correct in most cases, but doesn't work when comparing signed with unsigned operands. The solution proposed here tries to match a comparison of signed with unsigned operand, and perform Word32Compare simulation only in those cases. Unfortunately, the solution is not complete because it might skip cases where Word32 compare simulation is needed, so basically it is a hack. BUG= TEST=mjsunit/compiler/uint32 Committed: https://crrev.com/7499d92d7f4074c3ad7461ea70cbf896e30e6866 Cr-Commit-Position: refs/heads/master@{#40398} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7499d92d7f4074c3ad7461ea70cbf896e30e6866 Cr-Commit-Position: refs/heads/master@{#40398} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
