[turbofan] optimize redundant moves around splinter sites

When we split above an instruction (for example because of splintering),
we may introduce moves that are redundant in the context of
moves on subsequent instructions. Currently, we only detect such
redundancies by allowing moves to skip over Nop instructions (true
nops, with no input/output). We can also skip over other cases, for
example over constant definitions (nop with an output), since whatever
moves happen above it do not influence the instruction's outcome.

We may be able to handle other cases, too - in subsequent CLs.

BUG=

Committed: https://crrev.com/46878c1da1e8dbd386309ac12aad022f4e7761be
Cr-Commit-Position: refs/heads/master@{#31662}

[Mircea Trofin, 2015-10-29 02:09:45]

Description was changed from

==========
optimize moves

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

to

==========
When we split above an instruction (for example because of splintering),
we may introduce moves that are redundant in the context of 
moves on subsequent instructions. Currently, we only detect such 
redundancies by allowing moves to skip over Nop instructions (true
nops, with no input/output). We can also skip over other cases, for
example over constant definitions (nop with an output), since whatever
moves happen above it do not influence the instruction's outcome.

We may be able to handle other cases, too - in subsequent CLs.

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

[Mircea Trofin, 2015-10-29 02:09:45]

mtrofin@chromium.org changed reviewers:
+ bmeurer@chromium.org, jarin@chromium.org

[Jarin, 2015-10-29 07:37:25]

looking good, but I am not a big fan of the global Print functions.

https://codereview.chromium.org/1422333003/diff/40001/src/compiler/register-a...
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/1422333003/diff/40001/src/compiler/register-a...
src/compiler/register-allocator.h:809: void Print(const RegisterConfiguration*
config, const SpillRange* spill_range);
Why did you move the printing out of the class? We generally do not like global
functions.

[Mircea Trofin, 2015-10-29 14:14:40]

https://codereview.chromium.org/1422333003/diff/40001/src/compiler/register-a...
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/1422333003/diff/40001/src/compiler/register-a...
src/compiler/register-allocator.h:809: void Print(const RegisterConfiguration*
config, const SpillRange* spill_range);
On 2015/10/29 07:37:25, Jarin wrote:
> Why did you move the printing out of the class? We generally do not like
global
> functions.

I needed to visualize a few things (InstructionSequence, MoveOperands) in the
move optimization unit tests, 
where we don't have a RegisterAllocationData. All that I really need for the
prints is the RegisterConfiguration.

I didn't want to teach RegisterConfiguration about all these other concepts, and
I also didn't want to teach 
the rest of the model about RegisterConfiguration.

I think printing/diagnostics needs a cleanup - we have printf, operator<<, and
now these Print (regardless where
they live). I think the javascript object model also has some mechanism. I'd
want to take a shot at it, but once
the higher priority items clear up. Would it be OK if we live the Prints here
for now - they help with the unittest 
scenario I mentioned?

[Jarin, 2015-10-29 14:32:13]

https://codereview.chromium.org/1422333003/diff/40001/src/compiler/register-a...
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/1422333003/diff/40001/src/compiler/register-a...
src/compiler/register-allocator.h:809: void Print(const RegisterConfiguration*
config, const SpillRange* spill_range);
On 2015/10/29 14:14:40, Mircea Trofin wrote:
> On 2015/10/29 07:37:25, Jarin wrote:
> > Why did you move the printing out of the class? We generally do not like
> global
> > functions.
> 
> I needed to visualize a few things (InstructionSequence, MoveOperands) in the
> move optimization unit tests, 
> where we don't have a RegisterAllocationData. All that I really need for the
> prints is the RegisterConfiguration.

I understand, but why can't you move the printing into the class that it is
printing? That is generally what the JavaScript objects are doing.

In any case, could we do the printing in a separate CL? It seems to be quite
unrelated to the title of the CL.

> I didn't want to teach RegisterConfiguration about all these other concepts,
and
> I also didn't want to teach 
> the rest of the model about RegisterConfiguration.

I suppose this sort of answers the question above, but I think it is ok to teach
the rest of the model. 
 
> I think printing/diagnostics needs a cleanup - we have printf, operator<<, and
> now these Print (regardless where
> they live). I think the javascript object model also has some mechanism. I'd
> want to take a shot at it, but once
> the higher priority items clear up. Would it be OK if we live the Prints here
> for now - they help with the unittest 
> scenario I mentioned?

Hmm, I am really worried that it will stay and I would like to avoid another way
of printing (and also polluting the namespace with globals). 

We know too well that printing is a mess, and I do not believe that introducing
yet another way solves anything. A year ago we had a big push towards using
streams and most team members (including me) now think it was a mistake. You
would have hard time convincing anyone about a new way. I think you might have
some success to convince people about undoing the stream stuff (because it sucks
for bunch of reasons, most notably for debugging, but also for formatting
integers and such).

[Mircea Trofin, 2015-10-29 14:36:30]

On 2015/10/29 14:32:13, Jarin wrote:
>
https://codereview.chromium.org/1422333003/diff/40001/src/compiler/register-a...
> File src/compiler/register-allocator.h (right):
> 
>
https://codereview.chromium.org/1422333003/diff/40001/src/compiler/register-a...
> src/compiler/register-allocator.h:809: void Print(const RegisterConfiguration*
> config, const SpillRange* spill_range);
> On 2015/10/29 14:14:40, Mircea Trofin wrote:
> > On 2015/10/29 07:37:25, Jarin wrote:
> > > Why did you move the printing out of the class? We generally do not like
> > global
> > > functions.
> > 
> > I needed to visualize a few things (InstructionSequence, MoveOperands) in
the
> > move optimization unit tests, 
> > where we don't have a RegisterAllocationData. All that I really need for the
> > prints is the RegisterConfiguration.
> 
> I understand, but why can't you move the printing into the class that it is
> printing? That is generally what the JavaScript objects are doing.
> 
> In any case, could we do the printing in a separate CL? It seems to be quite
> unrelated to the title of the CL.
> 
I agree - it does fit in a different CL. 
> > I didn't want to teach RegisterConfiguration about all these other concepts,
> and
> > I also didn't want to teach 
> > the rest of the model about RegisterConfiguration.
> 
> I suppose this sort of answers the question above, but I think it is ok to
teach
> the rest of the model. 
>  
> > I think printing/diagnostics needs a cleanup - we have printf, operator<<,
and
> > now these Print (regardless where
> > they live). I think the javascript object model also has some mechanism. I'd
> > want to take a shot at it, but once
> > the higher priority items clear up. Would it be OK if we live the Prints
here
> > for now - they help with the unittest 
> > scenario I mentioned?
> 
> Hmm, I am really worried that it will stay and I would like to avoid another
way
> of printing (and also polluting the namespace with globals). 
> 
> We know too well that printing is a mess, and I do not believe that
introducing
> yet another way solves anything. A year ago we had a big push towards using
> streams and most team members (including me) now think it was a mistake. You
> would have hard time convincing anyone about a new way. I think you might have
> some success to convince people about undoing the stream stuff (because it
sucks
> for bunch of reasons, most notably for debugging, but also for formatting
> integers and such).

Oh, I see - didn't have the background. Thanks!

[Mircea Trofin, 2015-10-29 14:42:51]

Undid the unrelated Print changes.

[Jarin, 2015-10-29 14:53:14]

lgtm

https://codereview.chromium.org/1422333003/diff/60001/test/unittests/compiler...
File test/unittests/compiler/move-optimizer-unittest.cc (right):

https://codereview.chromium.org/1422333003/diff/60001/test/unittests/compiler...
test/unittests/compiler/move-optimizer-unittest.cc:230: CHECK(inst1_start ==
nullptr || inst1_start->size() == 0);
Nit: In unit tests, we should always use EXPECT (or ASSERT if you want the test
to terminated immediately on failure) rather than CHECK.

I do not know why we have not spotted this before.

[Mircea Trofin, 2015-10-29 16:10:54]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2015-10-29 16:11:00]

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

[commit-bot: I haz the power, 2015-10-29 16:12:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-29 16:13:01]

Patchset 4 (id:??) landed as
https://crrev.com/46878c1da1e8dbd386309ac12aad022f4e7761be
Cr-Commit-Position: refs/heads/master@{#31662}