[turbofan] Fix operands for VisitDiv on Intel.

[turbofan] Fix operands for VisitDiv on Intel.

The idiv instruction has 2 registers as output. This needs to be
modeled so that the move optimizer won't incorrectly elide away
moves.

BUG=

Committed: https://crrev.com/1da4b88e8200172eab71bc71253a8adf0e08b466
Cr-Commit-Position: refs/heads/master@{#34978}

[Mircea Trofin, 2016-03-22 04:29:00]

Description was changed from

==========
[turbofan] Fix operands for VisitDiv on Intel.

The idiv instruction has 2 registers as output. This needs to be
modeled so that the move optimizer won't incorrectly elide away
moves.

BUG=
==========

to

==========
[turbofan] Fix operands for VisitDiv on Intel.

The idiv instruction has 2 registers as output. This needs to be
modeled so that the move optimizer won't incorrectly elide away
moves.

BUG=
==========

[Mircea Trofin, 2016-03-22 04:29:00]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org, titzer@chromium.org

[bradn, 2016-03-22 06:21:37]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2016-03-22 06:21:37]

lgtm, seems to fix the problem.

[bradn, 2016-03-22 06:22:09]

titzer, please review for src/compiler owners

[titzer, 2016-03-22 07:50:54]

lgtm

[titzer, 2016-03-22 08:01:44]

The CQ bit was checked by titzer@chromium.org

[commit-bot: I haz the power, 2016-03-22 08:01:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1818323002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1818323002/1

[commit-bot: I haz the power, 2016-03-22 08:03:43]

Description was changed from

==========
[turbofan] Fix operands for VisitDiv on Intel.

The idiv instruction has 2 registers as output. This needs to be
modeled so that the move optimizer won't incorrectly elide away
moves.

BUG=
==========

to

==========
[turbofan] Fix operands for VisitDiv on Intel.

The idiv instruction has 2 registers as output. This needs to be
modeled so that the move optimizer won't incorrectly elide away
moves.

BUG=
==========

[commit-bot: I haz the power, 2016-03-22 08:03:44]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-22 08:04:56]

Description was changed from

==========
[turbofan] Fix operands for VisitDiv on Intel.

The idiv instruction has 2 registers as output. This needs to be
modeled so that the move optimizer won't incorrectly elide away
moves.

BUG=
==========

to

==========
[turbofan] Fix operands for VisitDiv on Intel.

The idiv instruction has 2 registers as output. This needs to be
modeled so that the move optimizer won't incorrectly elide away
moves.

BUG=

Committed: https://crrev.com/1da4b88e8200172eab71bc71253a8adf0e08b466
Cr-Commit-Position: refs/heads/master@{#34978}
==========

[commit-bot: I haz the power, 2016-03-22 08:04:57]

Patchset 1 (id:??) landed as
https://crrev.com/1da4b88e8200172eab71bc71253a8adf0e08b466
Cr-Commit-Position: refs/heads/master@{#34978}

[Jakob Kummerow, 2016-04-27 12:08:54]

DBC: This CL touches VisitMod, not VisitDiv. 

Why does it not include a regression test?

[Mircea Trofin, 2016-04-28 20:29:07]

On 2016/04/27 12:08:54, Jakob wrote:
> DBC: This CL touches VisitMod, not VisitDiv. 
> 
> Why does it not include a regression test?

I'm thinking about what a meaningful regression test may be. I don't think it's
a very clear cut answer. The way I see it, the change fixed a specification bug:
the way we modeled a specific instruction (idiv) did not match its specification
- we omitted operands. It so happened we lived fine with it all this time, until
my move optimization work uncovered it.

To hit this issue the way it was hit "out there", we need a test that assumes
the register allocator behaves a certain way. I'd be weary about such a test.

To have had avoided this bug being introduced in the first place, we'd need a
specification checker.

My preference would be to rely on more fuzz testing pre-release, combined with
testing top "X" sites.

[titzer, 2016-04-29 07:48:46]

On 2016/04/28 20:29:07, Mircea Trofin wrote:
> On 2016/04/27 12:08:54, Jakob wrote:
> > DBC: This CL touches VisitMod, not VisitDiv. 
> > 
> > Why does it not include a regression test?
> 
> I'm thinking about what a meaningful regression test may be. I don't think
it's
> a very clear cut answer. The way I see it, the change fixed a specification
bug:
> the way we modeled a specific instruction (idiv) did not match its
specification
> - we omitted operands. It so happened we lived fine with it all this time,
until
> my move optimization work uncovered it.
> 
> To hit this issue the way it was hit "out there", we need a test that assumes
> the register allocator behaves a certain way. I'd be weary about such a test.
> 
> To have had avoided this bug being introduced in the first place, we'd need a
> specification checker.
> 
> My preference would be to rely on more fuzz testing pre-release, combined with
> testing top "X" sites.

You're right that it's tough to nail these register allocator bugs. In the past
what I've done is to try to write a cctest that tries many different
permutations of surrounding code (e.g. adjusting the number and location of
parameters) to try to get a wide net to hit the bug. When I get such a test that
successfully hits it, then we keep it. That doesn't guarantee the exact bug
won't reappear in the future, but gives a little boost to test coverage in the
area--worthwhile enough, IMO. That of course doesn't preclude any of the other
suggestions you made.

[Mircea Trofin, 2016-04-29 15:15:07]

On 2016/04/29 07:48:46, titzer wrote:
> On 2016/04/28 20:29:07, Mircea Trofin wrote:
> > On 2016/04/27 12:08:54, Jakob wrote:
> > > DBC: This CL touches VisitMod, not VisitDiv. 
> > > 
> > > Why does it not include a regression test?
> > 
> > I'm thinking about what a meaningful regression test may be. I don't think
> it's
> > a very clear cut answer. The way I see it, the change fixed a specification
> bug:
> > the way we modeled a specific instruction (idiv) did not match its
> specification
> > - we omitted operands. It so happened we lived fine with it all this time,
> until
> > my move optimization work uncovered it.
> > 
> > To hit this issue the way it was hit "out there", we need a test that
assumes
> > the register allocator behaves a certain way. I'd be weary about such a
test.
> > 
> > To have had avoided this bug being introduced in the first place, we'd need
a
> > specification checker.
> > 
> > My preference would be to rely on more fuzz testing pre-release, combined
with
> > testing top "X" sites.
> 
> You're right that it's tough to nail these register allocator bugs. In the
past
> what I've done is to try to write a cctest that tries many different
> permutations of surrounding code (e.g. adjusting the number and location of
> parameters) to try to get a wide net to hit the bug. When I get such a test
that
> successfully hits it, then we keep it. That doesn't guarantee the exact bug
> won't reappear in the future, but gives a little boost to test coverage in the
> area--worthwhile enough, IMO. That of course doesn't preclude any of the other
> suggestions you made.

Thanks - this got me thinking - I think we can completely decouple this from
regalloc. We just need the optimizer to run on a hand-allocated chunk of Intel
code sequence. I'll put something together.