MIPS:[turbofan] Improve boolean materialization compares.

MIPS:[turbofan] Improve boolean materialization compares.

Additionally, improve immediate operand matching for branches
to avoid duplicated constant loading.

TEST=
BUG=

[paul.l..., 2015-08-15 00:22:30]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-08-15 00:22:31]

Here are a few initial comments, I need to think more on this, but I will be
away all next week. Sorry for the delay.

https://codereview.chromium.org/1294933003/diff/1/src/compiler/mips64/code-ge...
File src/compiler/mips64/code-generator-mips64.cc (right):

https://codereview.chromium.org/1294933003/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1051: // false.
With your change, the above comment should be deleted.

https://codereview.chromium.org/1294933003/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1059: __ Movz(result, kScratchReg2,
kScratchReg);
I'm not sure this is a good idea anymore, due to the removal of these
instructions for r6, and the existing macro  expands to a branch in that case.
So we'd have twwo branches right in a row (only one would be taken), but we
would use space for both.

https://codereview.chromium.org/1294933003/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1071: if (!predicate) __
xori(result, result, 1);
OK, it must be too late on Friday night, but I am not groking this 'predicate'
variable and the Boolean result inversion. Let's talk about this offline......

[paul.l..., 2015-08-15 00:23:41]

Also a nit: in commit message: impove -> improve

[paul.l..., 2015-09-03 22:27:40]

I really like materializing the booleans simply like this, without using a
branch. Especially since most of the Branch's will use Slt() anyway :-)

However, I must say that I had a hard time following the code at first read. I
had to really study it to derive the meaning. I think this is a sign that it
should be simplified/refactored a bit. Some commenting can help, but I have
other ideas as well. I will send you a separate CL later that maybe can help
with this.

https://codereview.chromium.org/1294933003/diff/40001/src/compiler/mips/code-...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/1294933003/diff/40001/src/compiler/mips/code-...
src/compiler/mips/code-generator-mips.cc:203: Condition
FlagsConditionToConditionCmp(bool& predicate,
It took me some staring at the code to figure out what this was doing, as it was
not obvious to me from the word 'predicate' what the function is. I did figure
out by reading 'AssembleArchBoolean', that you are basically making a decision
on whether a given test 'includes' equality, so that you can properly order
things for Slt() usage.

I think at the minimum, this needs commenting. I'd prefer a better name than
'predicate', but I cannot think of one.

However, I am looking at a refactoring that would get you derive this from the
returned condition code itself. I will send you separate email & CL. If at all
possible, I'd like to get away from the 'reference parameter',and the effecitive
need to return 2 things from each of these functions. Among other reasons, that
forces AssembleArchBranch(), which does not care about this, to have to create &
pass a fake parameter.

As a nit: for the eq/ne condition you set predicate true to equal, but for all
other conditions, predicate is true for the case that does _not_ include
equality: lt, gt, hi, lo, etc.

https://codereview.chromium.org/1294933003/diff/40001/src/compiler/mips/code-...
src/compiler/mips/code-generator-mips.cc:982: __ xori(kScratchReg2, zero_reg,
1);  // Create 1 for true.
Prefer li(kScratchReg2, 1) here, for readability. Same number of instructions.

https://codereview.chromium.org/1294933003/diff/40001/src/compiler/mips/code-...
src/compiler/mips/code-generator-mips.cc:983: if (!IsMipsArchVariant(kMips32r6))
{
I find that 'negative-conditions' -- if (not-something) -- provide just a little
bit of cognitive dissonance in trying to understand the meaning. Of course it is
sometimes necessary, but here you have the 'else' clause as well, so please
re-order the code to _start_ with the mips32r6 clause, for readability. One more
below as well.

https://codereview.chromium.org/1294933003/diff/40001/src/compiler/mips/code-...
src/compiler/mips/code-generator-mips.cc:986: __ Movz(result, kScratchReg2,
kScratchReg);
If you refactor the code a bit, you can get by with a single MovX instruction
for the mips32r2 case. Start by loading result with 1, then for the 'eq' case,
you can do a single __ Movz(result, zero_reg, kScratchReg);

(In r6 case you do need to preload kScratchReg2 with 1, not result, so it does
need some refactoring)

Hmmm, now that I look at this, isn't this backwards as written? IIUC, for the
'eq' case, predicate==true,  you return zero if the result of the And()
operation was non-zero. Don't we want to materialize 1 in that case?

[paul.l..., 2015-09-04 03:33:58]

Another round of comments, I was over-complicating my proposed solution, we can
just look at the cc itself to make decisions. 

Please see  https://codereview.chromium.org/1320073004

https://codereview.chromium.org/1294933003/diff/40001/src/compiler/mips/code-...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/1294933003/diff/40001/src/compiler/mips/code-...
src/compiler/mips/code-generator-mips.cc:203: Condition
FlagsConditionToConditionCmp(bool& predicate,
OK, I think we both overcomplicated this. When I started updating the code
(using a condition code refactoring that is no longer needed), I realized we
have everything right in the 'cc' itself. There is no need for this 'predicate'
(or the derivation from cc I had planned to do): All of this is an unnecessary
indirection.

Please see my changes in https://codereview.chromium.org/1320073004

(I did not remove predicate gneration from this function, but I do not use it in
ArchBoolean anymore, except in the FP case, which I did not examine yet.

https://codereview.chromium.org/1294933003/diff/40001/src/compiler/mips/code-...
src/compiler/mips/code-generator-mips.cc:986: __ Movz(result, kScratchReg2,
kScratchReg);
On 2015/09/03 22:27:39, paul.l... wrote:
> Hmmm, now that I look at this, isn't this backwards as written? IIUC, for the
> 'eq' case, predicate==true,  you return zero if the result of the And()
> operation was non-zero. Don't we want to materialize 1 in that case?

Never mind the last paragraph of prev comment (quoted here). First 2 para;s are
OK.

[dusmil.imgtec, 2015-09-04 12:57:48]

Thank you for review!  The code is improved and simpler now.
All comments addressed.