MIPS:[turbofan] Improve boolean materialization compares.

src/compiler/mips/code-generator-mips.cc

Index: src/compiler/mips/code-generator-mips.cc
diff --git a/src/compiler/mips/code-generator-mips.cc b/src/compiler/mips/code-generator-mips.cc
index fd6885b7555aa26ed6ca18573d730f5a5480f864..6eadbde18c0558f35903b20854e56b3c1b6747e8 100644
--- a/src/compiler/mips/code-generator-mips.cc
+++ b/src/compiler/mips/code-generator-mips.cc
@@ -200,32 +200,45 @@ class OutOfLineCeil final : public OutOfLineRound {
 };
 
 
-Condition FlagsConditionToConditionCmp(FlagsCondition condition) {
+Condition FlagsConditionToConditionCmp(bool& predicate,

[paul.l..., 2015/09/03 22:27:39]

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.

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

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.

+                                       FlagsCondition condition) {
   switch (condition) {
     case kEqual:
+      predicate = true;
       return eq;
     case kNotEqual:
+      predicate = false;
       return ne;
     case kSignedLessThan:
+      predicate = true;
       return lt;
     case kSignedGreaterThanOrEqual:
+      predicate = false;
       return ge;
     case kSignedLessThanOrEqual:
+      predicate = false;
       return le;
     case kSignedGreaterThan:
+      predicate = true;
       return gt;
     case kUnsignedLessThan:
+      predicate = true;
       return lo;
     case kUnsignedGreaterThanOrEqual:
+      predicate = false;
       return hs;
     case kUnsignedLessThanOrEqual:
+      predicate = false;
       return ls;
     case kUnsignedGreaterThan:
+      predicate = true;
       return hi;
     case kUnorderedEqual:
     case kUnorderedNotEqual:
+      predicate = true;
       break;
     default:
+      predicate = true;
       break;
   }
   UNREACHABLE();
@@ -233,13 +246,17 @@ Condition FlagsConditionToConditionCmp(FlagsCondition condition) {
 }
 
 
-Condition FlagsConditionToConditionTst(FlagsCondition condition) {
+Condition FlagsConditionToConditionTst(bool& predicate,
+                                       FlagsCondition condition) {
   switch (condition) {
     case kNotEqual:
+      predicate = false;
       return ne;
     case kEqual:
+      predicate = true;
       return eq;
     default:
+      predicate = true;
       break;
   }
   UNREACHABLE();
@@ -247,13 +264,17 @@ Condition FlagsConditionToConditionTst(FlagsCondition condition) {
 }
 
 
-Condition FlagsConditionToConditionOvf(FlagsCondition condition) {
+Condition FlagsConditionToConditionOvf(bool& predicate,
+                                       FlagsCondition condition) {
   switch (condition) {
     case kOverflow:
+      predicate = true;
       return lt;
     case kNotOverflow:
+      predicate = false;
       return ge;
     default:
+      predicate = true;
       break;
   }
   UNREACHABLE();
@@ -890,7 +911,7 @@ void CodeGenerator::AssembleArchBranch(Instruction* instr, BranchInfo* branch) {
   Label* tlabel = branch->true_label;
   Label* flabel = branch->false_label;
   Condition cc = kNoCondition;
-
+  bool predicate;
   // MIPS does not have condition code flags, so compare and branch are
   // implemented differently than on the other arch's. The compare operations
   // emit mips pseudo-instructions, which are handled here by branch
@@ -901,16 +922,16 @@ void CodeGenerator::AssembleArchBranch(Instruction* instr, BranchInfo* branch) {
   //    not separated by other instructions.
 
   if (instr->arch_opcode() == kMipsTst) {
-    cc = FlagsConditionToConditionTst(branch->condition);
+    cc = FlagsConditionToConditionTst(predicate, branch->condition);
     __ And(at, i.InputRegister(0), i.InputOperand(1));
     __ Branch(tlabel, cc, at, Operand(zero_reg));
   } else if (instr->arch_opcode() == kMipsAddOvf ||
              instr->arch_opcode() == kMipsSubOvf) {
     // kMipsAddOvf, SubOvf emit negative result to 'kCompareReg' on overflow.
-    cc = FlagsConditionToConditionOvf(branch->condition);
+    cc = FlagsConditionToConditionOvf(predicate, branch->condition);
     __ Branch(tlabel, cc, kCompareReg, Operand(zero_reg));
   } else if (instr->arch_opcode() == kMipsCmp) {
-    cc = FlagsConditionToConditionCmp(branch->condition);
+    cc = FlagsConditionToConditionCmp(predicate, branch->condition);
     __ Branch(tlabel, cc, i.InputRegister(0), i.InputOperand(1));
   } else if (instr->arch_opcode() == kMipsCmpS) {
     if (!convertCondition(branch->condition, cc)) {
@@ -950,41 +971,110 @@ void CodeGenerator::AssembleArchBoolean(Instruction* instr,
   DCHECK_NE(0u, instr->OutputCount());
   Register result = i.OutputRegister(instr->OutputCount() - 1);
   Condition cc = kNoCondition;
-
+  bool predicate;
   // MIPS does not have condition code flags, so compare and branch are
   // implemented differently than on the other arch's. The compare operations
   // emit mips psuedo-instructions, which are checked and handled here.
 
-  // For materializations, we use delay slot to set the result true, and
-  // in the false case, where we fall thru the branch, we reset the result
-  // false.
-
   if (instr->arch_opcode() == kMipsTst) {
-    cc = FlagsConditionToConditionTst(condition);
-    __ And(at, i.InputRegister(0), i.InputOperand(1));
-    __ Branch(USE_DELAY_SLOT, &done, cc, at, Operand(zero_reg));
-    __ li(result, Operand(1));  // In delay slot.
-
+    cc = FlagsConditionToConditionTst(predicate, condition);
+    __ And(kScratchReg, i.InputRegister(0), i.InputOperand(1));
+    __ xori(kScratchReg2, zero_reg, 1);  // Create 1 for true.

[paul.l..., 2015/09/03 22:27:39]

Prefer li(kScratchReg2, 1) here, for readability. Same number of instructions.

+    if (!IsMipsArchVariant(kMips32r6)) {

[paul.l..., 2015/09/03 22:27:39]

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.

+      if (predicate) {
+        __ Movn(result, zero_reg, kScratchReg);
+        __ Movz(result, kScratchReg2, kScratchReg);

[paul.l..., 2015/09/03 22:27:39]

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]

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

+      } else {
+        __ Movz(result, zero_reg, kScratchReg);
+        __ Movn(result, kScratchReg2, kScratchReg);
+      }
+    } else {
+      if (predicate) {
+        __ seleqz(result, kScratchReg2, kScratchReg);
+      } else {
+        __ selnez(result, kScratchReg2, kScratchReg);
+      }
+    }
+    return;
   } else if (instr->arch_opcode() == kMipsAddOvf ||
              instr->arch_opcode() == kMipsSubOvf) {
     // kMipsAddOvf, SubOvf emits negative result to 'kCompareReg' on overflow.
-    cc = FlagsConditionToConditionOvf(condition);
-    __ Branch(USE_DELAY_SLOT, &done, cc, kCompareReg, Operand(zero_reg));
-    __ li(result, Operand(1));  // In delay slot.
-
+    cc = FlagsConditionToConditionOvf(predicate, condition);
+    // Return 1 on overflow.
+    __ Slt(result, kCompareReg, Operand(zero_reg));
+    if (!predicate)  // Invert result on not overflow.
+      __ xori(result, result, 1);
+    return;
   } else if (instr->arch_opcode() == kMipsCmp) {
-    Register left = i.InputRegister(0);
-    Operand right = i.InputOperand(1);
-    cc = FlagsConditionToConditionCmp(condition);
-    __ Branch(USE_DELAY_SLOT, &done, cc, left, right);
-    __ li(result, Operand(1));  // In delay slot.
-
+    cc = FlagsConditionToConditionCmp(predicate, condition);
+    switch (cc) {
+      case eq:
+      case ne: {
+        Register left = i.InputRegister(0);
+        Operand right = i.InputOperand(1);
+        __ Subu(kScratchReg, left, right);
+        __ xori(kScratchReg2, zero_reg, 1);
+        if (!IsMipsArchVariant(kMips32r6)) {
+          if (predicate) {
+            __ Movn(result, zero_reg, kScratchReg);
+            __ Movz(result, kScratchReg2, kScratchReg);
+          } else {
+            __ Movz(result, zero_reg, kScratchReg);
+            __ Movn(result, kScratchReg2, kScratchReg);
+          }
+        } else {
+          if (predicate) {
+            __ seleqz(result, kScratchReg2, kScratchReg);
+          } else {
+            __ selnez(result, kScratchReg2, kScratchReg);
+          }
+        }
+      } break;
+      case lt:
+      case ge: {
+        Register left = i.InputRegister(0);
+        Operand right = i.InputOperand(1);
+        __ Slt(result, left, right);
+        if (!predicate) {
+          __ xori(result, result, 1);
+        }
+      } break;
+      case gt:
+      case le: {
+        Register left = i.InputRegister(1);
+        Operand right = i.InputOperand(0);
+        __ Slt(result, left, right);
+        if (!predicate) {
+          __ xori(result, result, 1);
+        }
+      } break;
+      case lo:
+      case hs: {
+        Register left = i.InputRegister(0);
+        Operand right = i.InputOperand(1);
+        __ Sltu(result, left, right);
+        if (!predicate) {
+          __ xori(result, result, 1);
+        }
+      } break;
+      case hi:
+      case ls: {
+        Register left = i.InputRegister(1);
+        Operand right = i.InputOperand(0);
+        __ Sltu(result, left, right);
+        if (!predicate) {
+          __ xori(result, result, 1);
+        }
+      } break;
+      default:
+        UNREACHABLE();
+    }
+    return;
   } else if (instr->arch_opcode() == kMipsCmpD ||
              instr->arch_opcode() == kMipsCmpS) {
     FPURegister left = i.InputDoubleRegister(0);
     FPURegister right = i.InputDoubleRegister(1);
 
-    bool predicate;
     FPUCondition cc = FlagsConditionToConditionCmpFPU(predicate, condition);
     if (!IsMipsArchVariant(kMips32r6)) {
       __ li(result, Operand(1));
@@ -1018,11 +1108,6 @@ void CodeGenerator::AssembleArchBoolean(Instruction* instr,
     TRACE_UNIMPL();
     UNIMPLEMENTED();
   }
-
-  // Fallthrough case is the false materialization.
-  __ bind(&false_value);
-  __ li(result, Operand(0));
-  __ bind(&done);
 }