[turbofan] optimize moves into merges

[turbofan] optimize moves into merges

BUG=

Committed: https://crrev.com/95df1bc266d66df225a145d93a591d955d3d707c
Cr-Commit-Position: refs/heads/master@{#26819}

[Benedikt Meurer, 2015-01-28 05:18:50]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-01-28 05:18:51]

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
File src/compiler/move-optimizer.cc (right):

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:13: struct MoveKey {
Once the issues mentioned below are addressed, how about getting rid of this
struct and using std::pair<InstructionOperand, InstructionOperand> instead?

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:18: struct OperandLess {
Can you add an operator< (and operator>) to InstructionOperand instead?

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:34: typedef std::set<InstructionOperand,
std::less<InstructionOperand>,
I guess this shouldn't be std::less<InstructionOperand>, but rather OperandLess?

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:36: typedef std::map<MoveKey, unsigned,
MoveKeyLess,
How about adding ZoneSet and ZoneMap to zone-containers.h and getting rid of
these typedefs, and replacing the classes below with simple typedefs?

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:135: static bool GapsCanMoveOver(Instruction*
instr) {
Nit: use anonymous namespace instead of static.

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:141: static int
FindFirstNonEmptySlot(GapInstruction* gap) {
Nit: use anonymous namespace instead of static.

[dcarney, 2015-02-02 09:23:33]

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
File src/compiler/move-optimizer.cc (right):

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:13: struct MoveKey {
On 2015/01/28 05:18:51, Benedikt Meurer wrote:
> Once the issues mentioned below are addressed, how about getting rid of this
> struct and using std::pair<InstructionOperand, InstructionOperand> instead?

Done.

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:18: struct OperandLess {
On 2015/01/28 05:18:51, Benedikt Meurer wrote:
> Can you add an operator< (and operator>) to InstructionOperand instead?

now that it's 64 bits, it might be a little wierd, since we only care about kind
and index... i can add them though if you want.

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:34: typedef std::set<InstructionOperand,
std::less<InstructionOperand>,
On 2015/01/28 05:18:51, Benedikt Meurer wrote:
> I guess this shouldn't be std::less<InstructionOperand>, but rather
OperandLess?

Done.

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:36: typedef std::map<MoveKey, unsigned,
MoveKeyLess,
On 2015/01/28 05:18:51, Benedikt Meurer wrote:
> How about adding ZoneSet and ZoneMap to zone-containers.h and getting rid of
> these typedefs, and replacing the classes below with simple typedefs?

Done.

https://codereview.chromium.org/755323011/diff/240001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:141: static int
FindFirstNonEmptySlot(GapInstruction* gap) {
On 2015/01/28 05:18:51, Benedikt Meurer wrote:
> Nit: use anonymous namespace instead of static.

Done.

[Benedikt Meurer, 2015-02-24 12:03:59]

LGTM, nice work.

[dcarney, 2015-02-24 12:06:07]

The CQ bit was checked by dcarney@chromium.org

[commit-bot: I haz the power, 2015-02-24 12:06:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/755323011/320001

[dcarney, 2015-02-24 12:11:35]

The CQ bit was unchecked by dcarney@chromium.org

[dcarney, 2015-02-24 12:12:00]

The CQ bit was checked by dcarney@chromium.org

[dcarney, 2015-02-24 12:12:00]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/755323011/#ps340001
(title: "fix")

[commit-bot: I haz the power, 2015-02-24 12:12:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/755323011/340001

[commit-bot: I haz the power, 2015-02-24 12:49:32]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-02-24 12:49:40]

Patchset 18 (id:??) landed as
https://crrev.com/95df1bc266d66df225a145d93a591d955d3d707c
Cr-Commit-Position: refs/heads/master@{#26819}

[brucedawson, 2015-04-06 18:05:30]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-04-06 18:05:31]

Please take a look at variable shadowing comment?

https://codereview.chromium.org/755323011/diff/340001/src/compiler/move-optim...
File src/compiler/move-optimizer.cc (right):

https://codereview.chromium.org/755323011/diff/340001/src/compiler/move-optim...
src/compiler/move-optimizer.cc:220: auto gap = LastGap(pred);
This line of code is worrisome because it shadows the 'gap' variable declared on
line 194. There is *probably* not a bug here, but I can't be sure -- the
references to 'gap' on line 237 and 238 make me wonder whether the assignment on
line 220 is supposed to affect them or not.

Regardless of whether or not there is a bug, I think it is worth changing the
variable name to avoid the ambiguity, if only to make the code less confusing
for future readers.

This was found as a new warning by the VC++ /analyze builder.

The warning disappeared for a few days which is why it is now reported as being
'new'.

Bug 427616 is the catch-all bug for bugs/changes related to /analyze.

[dcarney, 2015-04-06 18:45:12]

On 2015/04/06 18:05:31, brucedawson wrote:
> Please take a look at variable shadowing comment?
> 
>
https://codereview.chromium.org/755323011/diff/340001/src/compiler/move-optim...
> File src/compiler/move-optimizer.cc (right):
> 
>
https://codereview.chromium.org/755323011/diff/340001/src/compiler/move-optim...
> src/compiler/move-optimizer.cc:220: auto gap = LastGap(pred);
> This line of code is worrisome because it shadows the 'gap' variable declared
on
> line 194. There is *probably* not a bug here, but I can't be sure -- the
> references to 'gap' on line 237 and 238 make me wonder whether the assignment
on
> line 220 is supposed to affect them or not.
> 
> Regardless of whether or not there is a bug, I think it is worth changing the
> variable name to avoid the ambiguity, if only to make the code less confusing
> for future readers.

it's not a bug, but i'll fix it

[Sven Panne, 2015-04-07 07:15:50]

On 2015/04/06 18:05:31, brucedawson wrote:
> [...] Bug 427616 is the catch-all bug for bugs/changes related to /analyze.

Do we have a bot for /analyze? Looking at the output might be helpful.

[brucedawson, 2015-04-07 17:06:18]

We do have a /analyze bot at
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Windows%20Analyze/.

However, it displays the raw results which are a bit daunting: 6.5 MB of raw
text. After running through a script that throws away duplicates and organizes
the warnings it is still 733 KB. I look at new warnings each morning. At some
point I will check in and document the necessary scripts so that anybody can do
this. However, given the high signal-to-noise ratio (many confusingly described
false positives) I don't really recommend looking at the results.