MIPS: Fix bad RegisterConfiguration usage in InstructionSequence unit tests.

MIPS: Fix bad RegisterConfiguration usage in InstructionSequence unit tests.

Test InstructionSequenceTest has been initialized with a testing RegisterConfiguration
instance defined in instruction-sequence-unittest.h, whereas class ExplicitOperand which
is being tested used RegisterConfiguration from instruction.cc. In case these two
instances are different, the tests would fail. The issue is fixed by using the same
instance of RegisterConfiguration both for test code and code under test.

Additionally, the tests in register-allocator-unittest.cc use hardcoded values
for register and begin failing is the hardcoded register is not available for
allocation. Fix by forcing the use of allocatable registers only.

TEST=unittests.MoveOptimizerTest.RemovesRedundantExplicit,unittests.RegisterAllocatorTest.SpillPhi
BUG=

Committed: https://crrev.com/0cf56232209d4c9c669b8426680de18806f6c29a
Cr-Commit-Position: refs/heads/master@{#40862}

[ivica.bogosavljevic, 2016-10-19 14:30:39]

ivica.bogosavljevic@imgtec.com changed reviewers:
+ bmeurer@chromium.org, dcarney@chromium.org

[ivica.bogosavljevic, 2016-10-19 14:30:40]

PTAL

Notice that this is a draft, not a final solution. I needed to define external
variable InstructionSequenceTest code in order to access variables not exported
in InstructionSequence.

Also, probably the fix for RegisterAllocatorTest.SpillPhi should be applied to
all tests in that suite.

https://codereview.chromium.org/2433093002/diff/1/src/compiler/instruction.cc
File src/compiler/instruction.cc (right):

https://codereview.chromium.org/2433093002/diff/1/src/compiler/instruction.cc...
src/compiler/instruction.cc:16: RegisterConfiguration::Turbofan;
Needed to fix the type in order to use it from the testing code

https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/ins...
File test/unittests/compiler/instruction-sequence-unittest.cc (right):

https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/ins...
test/unittests/compiler/instruction-sequence-unittest.cc:83: extern const
RegisterConfiguration* (*GetRegConfig)();
Ugly but couldn't think of any better. If you have any ideas please propose them

https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/reg...
File test/unittests/compiler/register-allocator-unittest.cc (right):

https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/reg...
test/unittests/compiler/register-allocator-unittest.cc:323: auto left =
Define(Reg(GetAllocatableCode(0)));
I guess the same thing should be done everywhere. That's why I wanted to add
this code to the definition of InstructionSequenceTest::Reg, but that method is
static so it wasn't possible.

[ivica.bogosavljevic, 2016-10-25 07:18:11]

Reminder...

[ivica.bogosavljevic, 2016-10-28 08:17:23]

ivica.bogosavljevic@imgtec.com changed reviewers:
+ mstarzinger@chromium.org, vogelheim@chromium.org

[ivica.bogosavljevic, 2016-11-03 12:51:57]

ivica.bogosavljevic@imgtec.com changed reviewers:
+ titzer@chromium.org

[ivica.bogosavljevic, 2016-11-03 12:51:58]

Reminder...

[titzer, 2016-11-03 13:00:12]

https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/ins...
File test/unittests/compiler/instruction-sequence-unittest.cc (right):

https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/ins...
test/unittests/compiler/instruction-sequence-unittest.cc:83: extern const
RegisterConfiguration* (*GetRegConfig)();
On 2016/10/19 14:30:40, ivica.bogosavljevic wrote:
> Ugly but couldn't think of any better. If you have any ideas please propose
them

How about adding a separate method that has a XXXForTesting name and is
accessible from tests?

[ivica.bogosavljevic, 2016-11-03 15:22:54]

Uploaded a new CL, PTAL

https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/ins...
File test/unittests/compiler/instruction-sequence-unittest.cc (right):

https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/ins...
test/unittests/compiler/instruction-sequence-unittest.cc:83: extern const
RegisterConfiguration* (*GetRegConfig)();
On 2016/11/03 13:00:12, titzer wrote:
> On 2016/10/19 14:30:40, ivica.bogosavljevic wrote:
> > Ugly but couldn't think of any better. If you have any ideas please propose
> them
> 
> How about adding a separate method that has a XXXForTesting name and is
> accessible from tests?

Good idea!

[titzer, 2016-11-07 09:53:13]

On 2016/11/03 15:22:54, ivica.bogosavljevic wrote:
> Uploaded a new CL, PTAL
> 
>
https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/ins...
> File test/unittests/compiler/instruction-sequence-unittest.cc (right):
> 
>
https://codereview.chromium.org/2433093002/diff/1/test/unittests/compiler/ins...
> test/unittests/compiler/instruction-sequence-unittest.cc:83: extern const
> RegisterConfiguration* (*GetRegConfig)();
> On 2016/11/03 13:00:12, titzer wrote:
> > On 2016/10/19 14:30:40, ivica.bogosavljevic wrote:
> > > Ugly but couldn't think of any better. If you have any ideas please
propose
> > them
> > 
> > How about adding a separate method that has a XXXForTesting name and is
> > accessible from tests?
> 
> Good idea!

lgtm

[ivica.bogosavljevic, 2016-11-09 07:28:57]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com

[commit-bot: I haz the power, 2016-11-09 07:29:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 07:30:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 07:30:11]

Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15851)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12147)

[ivica.bogosavljevic, 2016-11-09 13:10:50]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 13:10:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 13:36:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 13:36:43]

Dry run: This issue passed the CQ dry run.

[ivica.bogosavljevic, 2016-11-09 13:45:32]

The CQ bit was checked by ivica.bogosavljevic@imgtec.com

[ivica.bogosavljevic, 2016-11-09 13:45:33]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2433093002/#ps40001
(title: "Rebase to master")

[commit-bot: I haz the power, 2016-11-09 13:45:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 13:47:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-17 22:27:33]

Description was changed from

==========
MIPS: Fix bad RegisterConfiguration usage in InstructionSequence unit tests.

Test InstructionSequenceTest has been initialized with a testing
RegisterConfiguration
instance defined in instruction-sequence-unittest.h, whereas class
ExplicitOperand which
is being tested used RegisterConfiguration from instruction.cc. In case these
two
instances are different, the tests would fail. The issue is fixed by using the
same
instance of RegisterConfiguration both for test code and code under test.

Additionally, the tests in register-allocator-unittest.cc use hardcoded values
for register and begin failing is the hardcoded register is not available for
allocation. Fix by forcing the use of allocatable registers only.

TEST=unittests.MoveOptimizerTest.RemovesRedundantExplicit,unittests.RegisterAllocatorTest.SpillPhi
BUG=
==========

to

==========
MIPS: Fix bad RegisterConfiguration usage in InstructionSequence unit tests.

Test InstructionSequenceTest has been initialized with a testing
RegisterConfiguration
instance defined in instruction-sequence-unittest.h, whereas class
ExplicitOperand which
is being tested used RegisterConfiguration from instruction.cc. In case these
two
instances are different, the tests would fail. The issue is fixed by using the
same
instance of RegisterConfiguration both for test code and code under test.

Additionally, the tests in register-allocator-unittest.cc use hardcoded values
for register and begin failing is the hardcoded register is not available for
allocation. Fix by forcing the use of allocatable registers only.

TEST=unittests.MoveOptimizerTest.RemovesRedundantExplicit,unittests.RegisterAllocatorTest.SpillPhi
BUG=

Committed: https://crrev.com/0cf56232209d4c9c669b8426680de18806f6c29a
Cr-Commit-Position: refs/heads/master@{#40862}
==========

[commit-bot: I haz the power, 2016-11-17 22:27:35]

Patchset 3 (id:??) landed as
https://crrev.com/0cf56232209d4c9c669b8426680de18806f6c29a
Cr-Commit-Position: refs/heads/master@{#40862}

[Mircea Trofin, 2016-12-16 22:39:24]

On 2016/11/17 22:27:35, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/0cf56232209d4c9c669b8426680de18806f6c29a
> Cr-Commit-Position: refs/heads/master@{#40862}

This change rendered InstructionSequenceTest::SetNumRegs ineffectual, thus
loosening the tests that were using that API to ensure correct register
allocation under intentionally constrained setups. 

For the problem stated in this CL, a solution needs to continue supporting the
intentionally set-up test configuration.

[Mircea Trofin, 2016-12-16 22:39:27]

On 2016/11/17 22:27:35, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/0cf56232209d4c9c669b8426680de18806f6c29a
> Cr-Commit-Position: refs/heads/master@{#40862}

This change rendered InstructionSequenceTest::SetNumRegs ineffectual, thus
loosening the tests that were using that API to ensure correct register
allocation under intentionally constrained setups. 

For the problem stated in this CL, a solution needs to continue supporting the
intentionally set-up test configuration.

[Mircea Trofin, 2016-12-16 22:41:50]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2587593002/ by mtrofin@chromium.org.

The reason for reverting is: This change rendered
InstructionSequenceTest::SetNumRegs ineffectual, thus
loosening the tests that were using that API to ensure correct register
allocation under intentionally constrained setups. 

For the problem stated in this CL, a solution needs to continue supporting the
intentionally set-up test configuration..

[ivica.bogosavljevic, 2016-12-21 15:50:42]

On 2016/12/16 22:41:50, Mircea Trofin wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/2587593002/ by mailto:mtrofin@chromium.org.
> 
> The reason for reverting is: This change rendered
> InstructionSequenceTest::SetNumRegs ineffectual, thus
> loosening the tests that were using that API to ensure correct register
> allocation under intentionally constrained setups. 
> 
> For the problem stated in this CL, a solution needs to continue supporting the
> intentionally set-up test configuration..

Hi Mircea!

The reason why this patch landed in the first place is that testing code and
code
under test were using different instances of RegisterConfiguration for testing.
Without the patch
ExplicitOperand uses the RegisterConfiguration from instruction.cc, whereas 
Instruction-Sequence-Unittest uses its own version of RegisterConfiguration.
Even when
all test pass, they pass by accident
(unittests.MoveOptimizerTest.RemovesRedundantExplicit fails on 
MIPS).

[Mircea Trofin, 2016-12-21 16:26:42]

On 2016/12/21 15:50:42, ivica.bogosavljevic wrote:
> On 2016/12/16 22:41:50, Mircea Trofin wrote:
> > A revert of this CL (patchset #3 id:40001) has been created in
> > https://codereview.chromium.org/2587593002/ by mailto:mtrofin@chromium.org.
> > 
> > The reason for reverting is: This change rendered
> > InstructionSequenceTest::SetNumRegs ineffectual, thus
> > loosening the tests that were using that API to ensure correct register
> > allocation under intentionally constrained setups. 
> > 
> > For the problem stated in this CL, a solution needs to continue supporting
the
> > intentionally set-up test configuration..
> 
> Hi Mircea!
> 
> The reason why this patch landed in the first place is that testing code and
> code
> under test were using different instances of RegisterConfiguration for
testing.
> Without the patch
> ExplicitOperand uses the RegisterConfiguration from instruction.cc, whereas 
> Instruction-Sequence-Unittest uses its own version of RegisterConfiguration.
> Even when
> all test pass, they pass by accident
> (unittests.MoveOptimizerTest.RemovesRedundantExplicit fails on 
> MIPS).

I understand. I was suggesting the solution should continue using the test
register configuration, meaning, getting ExplicitOperand to support the
injection of a test register configuration, akin to what the register allocator
does. Is this something you'd be interested in looking at?

Thanks!