https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc
File src/compiler/x64/instruction-selector-x64.cc (right):
https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc#newcode370
src/compiler/x64/instruction-selector-x64.cc:370: : public
ValueMatcher<int32_t, IrOpcode::kInt32Add> {
What do you inherit from ValueMatcher here?
https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc#newcode378
src/compiler/x64/instruction-selector-x64.cc:378:
MatchMemoryOperand(selector);
This method is only used in the constructor and I think inlining it
would be more readable.
https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc#newcode393
src/compiler/x64/instruction-selector-x64.cc:393: if (scale_factor_ ==
1) {
This pattern with switching on the scale factor could be shortened with
a subroutine like:
pickAddressingModeFromScale(scale_factor, AddressingMode[4] modes)
https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc#newcode456
src/compiler/x64/instruction-selector-x64.cc:456: static bool
TryCombine(InstructionSelector* selector, Node* use, Node* node,
This is tough to follow, partly because of the pointers to pointers.
E.g. see questions below.
https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc#newcode461
src/compiler/x64/instruction-selector-x64.cc:461: *constant = node;
Why would the constant already be non-null and why would it be ok to
fall through and return true?
https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc#newcode468
src/compiler/x64/instruction-selector-x64.cc:468: if (leftm.IsPowerOf2()
&& leftm.Value() <= 8) {
This can't happen because Int32BinopMatcher puts constants on the right
automatically for commutative operators, which Int32Mul should be.
https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc#newcode494
src/compiler/x64/instruction-selector-x64.cc:494: return false;
So we can return false if scaled and unscaled are set to non-null? What
are they supposed to be prior to the call to this method?
https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc#newcode546
src/compiler/x64/instruction-selector-x64.cc:546:
selector->CanCover(node, right)) {
What happens if we handled left already here?
https://codereview.chromium.org/704713003/diff/120001/src/compiler/x64/instruction-selector-x64.cc#newcode613
src/compiler/x64/instruction-selector-x64.cc:613: if
(TryLeaInt32Add(this, node)) return;
I think this would be more readable if it was inline too, since then you
don't need to pass around the InstructionSelector, too.
https://codereview.chromium.org/704713003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
