Merge "Fix code generation with materialized conditions."

9 files changed, 129 insertions, 21 deletions

diff --git a/compiler/optimizing/code_generator_arm.cc b/compiler/optimizing/code_generator_arm.cc
index ad622798a6..804d352e72 100644
--- a/compiler/optimizing/code_generator_arm.cc
+++ b/compiler/optimizing/code_generator_arm.cc
@@ -577,7 +577,7 @@ void LocationsBuilderARM::VisitIf(HIf* if_instr) {
   DCHECK(cond->IsCondition());
   HCondition* condition = cond->AsCondition();
   if (condition->NeedsMaterialization()) {
-    locations->SetInAt(0, Location::Any());
+    locations->SetInAt(0, Location::RequiresRegister());
   }
 }
 
@@ -590,7 +590,7 @@ void InstructionCodeGeneratorARM::VisitIf(HIf* if_instr) {
     DCHECK(if_instr->GetLocations()->InAt(0).IsRegister());
     __ cmp(if_instr->GetLocations()->InAt(0).AsArm().AsCoreRegister(),
            ShifterOperand(0));
-    __ b(codegen_->GetLabelOf(if_instr->IfTrueSuccessor()), EQ);
+    __ b(codegen_->GetLabelOf(if_instr->IfTrueSuccessor()), NE);
   } else {
     // Condition has not been materialized, use its inputs as the comparison and its
     // condition as the branch condition.
diff --git a/compiler/optimizing/code_generator_x86.cc b/compiler/optimizing/code_generator_x86.cc
index 3383cb2117..f7b849564d 100644
--- a/compiler/optimizing/code_generator_x86.cc
+++ b/compiler/optimizing/code_generator_x86.cc
@@ -541,14 +541,18 @@ void InstructionCodeGeneratorX86::VisitIf(HIf* if_instr) {
   DCHECK(cond->IsCondition());
   HCondition* condition = cond->AsCondition();
   if (condition->NeedsMaterialization()) {
-    // Materialized condition, compare against 0
-    Location lhs = if_instr->GetLocations()->InAt(0);
-    if (lhs.IsRegister()) {
-      __ cmpl(lhs.AsX86().AsCpuRegister(), Immediate(0));
-    } else {
-      __ cmpl(Address(ESP, lhs.GetStackIndex()), Immediate(0));
+    // Moves do not affect the eflags register, so if the condition is evaluated
+    // just before the if, we don't need to evaluate it again.
+    if (!condition->IsBeforeWhenDisregardMoves(if_instr)) {
+      // Materialized condition, compare against 0
+      Location lhs = if_instr->GetLocations()->InAt(0);
+      if (lhs.IsRegister()) {
+        __ cmpl(lhs.AsX86().AsCpuRegister(), Immediate(0));
+      } else {
+        __ cmpl(Address(ESP, lhs.GetStackIndex()), Immediate(0));
+      }
     }
-    __ j(kEqual,  codegen_->GetLabelOf(if_instr->IfTrueSuccessor()));
+    __ j(kNotEqual,  codegen_->GetLabelOf(if_instr->IfTrueSuccessor()));
   } else {
     Location lhs = condition->GetLocations()->InAt(0);
     Location rhs = condition->GetLocations()->InAt(1);
@@ -625,6 +629,9 @@ void LocationsBuilderX86::VisitCondition(HCondition* comp) {
 void InstructionCodeGeneratorX86::VisitCondition(HCondition* comp) {
   if (comp->NeedsMaterialization()) {
     LocationSummary* locations = comp->GetLocations();
+    Register reg = locations->Out().AsX86().AsCpuRegister();
+    // Clear register: setcc only sets the low byte.
+    __ xorl(reg, reg);
     if (locations->InAt(1).IsRegister()) {
       __ cmpl(locations->InAt(0).AsX86().AsCpuRegister(),
               locations->InAt(1).AsX86().AsCpuRegister());
@@ -636,7 +643,7 @@ void InstructionCodeGeneratorX86::VisitCondition(HCondition* comp) {
       __ cmpl(locations->InAt(0).AsX86().AsCpuRegister(),
               Address(ESP, locations->InAt(1).GetStackIndex()));
     }
-    __ setb(X86Condition(comp->GetCondition()), locations->Out().AsX86().AsCpuRegister());
+    __ setb(X86Condition(comp->GetCondition()), reg);
   }
 }
 
diff --git a/compiler/optimizing/code_generator_x86_64.cc b/compiler/optimizing/code_generator_x86_64.cc
index ca03af8e9f..283d850ef0 100644
--- a/compiler/optimizing/code_generator_x86_64.cc
+++ b/compiler/optimizing/code_generator_x86_64.cc
@@ -424,14 +424,18 @@ void InstructionCodeGeneratorX86_64::VisitIf(HIf* if_instr) {
   DCHECK(cond->IsCondition());
   HCondition* condition = cond->AsCondition();
   if (condition->NeedsMaterialization()) {
-    // Materialized condition, compare against 0.
-    Location lhs = if_instr->GetLocations()->InAt(0);
-    if (lhs.IsRegister()) {
-      __ cmpl(lhs.AsX86_64().AsCpuRegister(), Immediate(0));
-    } else {
-      __ cmpl(Address(CpuRegister(RSP), lhs.GetStackIndex()), Immediate(0));
+    // Moves do not affect the eflags register, so if the condition is evaluated
+    // just before the if, we don't need to evaluate it again.
+    if (!condition->IsBeforeWhenDisregardMoves(if_instr)) {
+      // Materialized condition, compare against 0.
+      Location lhs = if_instr->GetLocations()->InAt(0);
+      if (lhs.IsRegister()) {
+        __ cmpl(lhs.AsX86_64().AsCpuRegister(), Immediate(0));
+      } else {
+        __ cmpl(Address(CpuRegister(RSP), lhs.GetStackIndex()), Immediate(0));
+      }
     }
-    __ j(kEqual, codegen_->GetLabelOf(if_instr->IfTrueSuccessor()));
+    __ j(kNotEqual, codegen_->GetLabelOf(if_instr->IfTrueSuccessor()));
   } else {
     Location lhs = condition->GetLocations()->InAt(0);
     Location rhs = condition->GetLocations()->InAt(1);
@@ -505,6 +509,9 @@ void LocationsBuilderX86_64::VisitCondition(HCondition* comp) {
 void InstructionCodeGeneratorX86_64::VisitCondition(HCondition* comp) {
   if (comp->NeedsMaterialization()) {
     LocationSummary* locations = comp->GetLocations();
+    CpuRegister reg = locations->Out().AsX86_64().AsCpuRegister();
+    // Clear register: setcc only sets the low byte.
+    __ xorq(reg, reg);
     if (locations->InAt(1).IsRegister()) {
       __ cmpq(locations->InAt(0).AsX86_64().AsCpuRegister(),
               locations->InAt(1).AsX86_64().AsCpuRegister());
@@ -515,8 +522,7 @@ void InstructionCodeGeneratorX86_64::VisitCondition(HCondition* comp) {
       __ cmpq(locations->InAt(0).AsX86_64().AsCpuRegister(),
               Address(CpuRegister(RSP), locations->InAt(1).GetStackIndex()));
     }
-    __ setcc(X86_64Condition(comp->GetCondition()),
-             comp->GetLocations()->Out().AsX86_64().AsCpuRegister());
+    __ setcc(X86_64Condition(comp->GetCondition()), reg);
   }
 }
 
diff --git a/compiler/optimizing/graph_visualizer.cc b/compiler/optimizing/graph_visualizer.cc
index 7f64be42d4..0fb4737db2 100644
--- a/compiler/optimizing/graph_visualizer.cc
+++ b/compiler/optimizing/graph_visualizer.cc
@@ -82,6 +82,8 @@ class HGraphVisualizerPrinter : public HGraphVisitor {
   }
 
   char GetTypeId(Primitive::Type type) {
+    // Note that Primitive::Descriptor would not work for us
+    // because it does not handle reference types (that is kPrimNot).
     switch (type) {
       case Primitive::kPrimBoolean: return 'z';
       case Primitive::kPrimByte: return 'b';
@@ -127,6 +129,12 @@ class HGraphVisualizerPrinter : public HGraphVisitor {
       }
     } else if (location.IsConstant()) {
       output_ << "constant";
+      HConstant* constant = location.GetConstant();
+      if (constant->IsIntConstant()) {
+        output_ << " " << constant->AsIntConstant()->GetValue();
+      } else if (constant->IsLongConstant()) {
+        output_ << " " << constant->AsLongConstant()->GetValue();
+      }
     } else if (location.IsInvalid()) {
       output_ << "invalid";
     } else if (location.IsStackSlot()) {
diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc
index 72c5834a8c..09412a9c86 100644
--- a/compiler/optimizing/nodes.cc
+++ b/compiler/optimizing/nodes.cc
@@ -555,14 +555,22 @@ bool HCondition::NeedsMaterialization() const {
     return true;
   }
 
-  // TODO: should we allow intervening instructions with no side-effect between this condition
-  // and the If instruction?
+  // TODO: if there is no intervening instructions with side-effect between this condition
+  // and the If instruction, we should move the condition just before the If.
   if (GetNext() != user) {
     return true;
   }
   return false;
 }
 
+bool HCondition::IsBeforeWhenDisregardMoves(HIf* if_) const {
+  HInstruction* previous = if_->GetPrevious();
+  while (previous != nullptr && previous->IsParallelMove()) {
+    previous = previous->GetPrevious();
+  }
+  return previous == this;
+}
+
 bool HInstruction::Equals(HInstruction* other) const {
   if (!InstructionTypeEquals(other)) return false;
   DCHECK_EQ(GetKind(), other->GetKind());
diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index fd65338442..be6b355d22 100644
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -1095,8 +1095,15 @@ class HCondition : public HBinaryOperation {
       : HBinaryOperation(Primitive::kPrimBoolean, first, second) {}
 
   virtual bool IsCommutative() { return true; }
+
+  // For register allocation purposes, returns whether this instruction needs to be
+  // materialized (that is, not just be in the processor flags).
   bool NeedsMaterialization() const;
 
+  // For code generation purposes, returns whether this instruction is just before
+  // `if_`, and disregard moves in between.
+  bool IsBeforeWhenDisregardMoves(HIf* if_) const;
+
   DECLARE_INSTRUCTION(Condition);
 
   virtual IfCondition GetCondition() const = 0;
diff --git a/test/409-materialized-condition/expected.txt b/test/409-materialized-condition/expected.txt
new file mode 100644
index 0000000000..a0796cde77
--- /dev/null
+++ b/test/409-materialized-condition/expected.txt
@@ -0,0 +1,5 @@
+foo1
+In do nothing.
+In if.
+foo2
+In do nothing.
diff --git a/test/409-materialized-condition/info.txt b/test/409-materialized-condition/info.txt
new file mode 100644
index 0000000000..898560d9be
--- /dev/null
+++ b/test/409-materialized-condition/info.txt
@@ -0,0 +1 @@
+Test that materialized conditions are evaluated correctly.
diff --git a/test/409-materialized-condition/src/Main.java b/test/409-materialized-condition/src/Main.java
new file mode 100644
index 0000000000..0c179a99de
--- /dev/null
+++ b/test/409-materialized-condition/src/Main.java
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 2014 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class Main {
+
+  public static void doNothing(boolean b) {
+    System.out.println("In do nothing.");
+  }
+
+  public static void inIf() {
+    System.out.println("In if.");
+  }
+
+  public static int bar() {
+    return 42;
+  }
+
+  public static int foo1() {
+    int b = bar();
+    doNothing(b == 42);
+    // This second `b == 42` will be GVN'ed away.
+    if (b == 42) {
+      inIf();
+      return b;
+    }
+    return 0;
+  }
+
+  public static int foo2() {
+    int b = bar();
+    doNothing(b == 41);
+    // This second `b == 41` will be GVN'ed away.
+    if (b == 41) {
+      inIf();
+      return 0;
+    }
+    return b;
+  }
+
+  public static void main(String[] args) {
+    System.out.println("foo1");
+    int res = foo1();
+    if (res != 42) {
+      throw new Error("Unexpected return value for foo1: " + res + ", expected 42.");
+    }
+
+    System.out.println("foo2");
+    res = foo2();
+    if (res != 42) {
+      throw new Error("Unexpected return value for foo2: " + res + ", expected 42.");
+    }
+  }
+}