Fix issue with Partial LSE and casts/instanceof

If PartialLSE encounters an instanceof or check-cast before the
escapes it could not correctly handle it. Due to how java language
code is typically developed and compiled this is generally not a
problem but could lead to incorrect codegen on release builds or
DCHECK failures on debug builds. This fixes the issues by (1) causing
partial LSE to consider check-cast and instance-ofs to be escaping.
This also updates the instruction simplifier to be much more
aggressive in removing instance-of and check-casts.

Test: ./test.py --host
Bug: 186041085
Change-Id: Ia513c4210a87a0dfa92f10adc530e17ee631d006

1 files changed, 30 insertions, 20 deletions

diff --git a/compiler/optimizing/instruction_simplifier.cc b/compiler/optimizing/instruction_simplifier.cc
index 938a77563e..23a432ef9d 100644
--- a/compiler/optimizing/instruction_simplifier.cc
+++ b/compiler/optimizing/instruction_simplifier.cc
@@ -611,7 +611,16 @@ static bool TypeCheckHasKnownOutcome(ReferenceTypeInfo class_rti,
 
   if (!class_rti.IsValid()) {
     // Happens when the loaded class is unresolved.
-    return false;
+    if (obj_rti.IsExact()) {
+      // outcome == 'true' && obj_rti is valid implies that class_rti is valid.
+      // Since that's a contradiction we must not pass this check.
+      *outcome = false;
+      return true;
+    } else {
+      // We aren't able to say anything in particular since we don't know the
+      // exact type of the object.
+      return false;
+    }
   }
   DCHECK(class_rti.IsExact());
   if (class_rti.IsSupertypeOf(obj_rti)) {
@@ -633,12 +642,6 @@ static bool TypeCheckHasKnownOutcome(ReferenceTypeInfo class_rti,
 
 void InstructionSimplifierVisitor::VisitCheckCast(HCheckCast* check_cast) {
   HInstruction* object = check_cast->InputAt(0);
-  if (check_cast->GetTypeCheckKind() != TypeCheckKind::kBitstringCheck &&
-      check_cast->GetTargetClass()->NeedsAccessCheck()) {
-    // If we need to perform an access check we cannot remove the instruction.
-    return;
-  }
-
   if (CanEnsureNotNullAt(object, check_cast)) {
     check_cast->ClearMustDoNullCheck();
   }
@@ -649,6 +652,11 @@ void InstructionSimplifierVisitor::VisitCheckCast(HCheckCast* check_cast) {
     return;
   }
 
+  // Minor correctness check.
+  DCHECK(check_cast->GetTargetClass()->StrictlyDominates(check_cast))
+      << "Illegal graph!\n"
+      << check_cast->DumpWithArgs();
+
   // Historical note: The `outcome` was initialized to please Valgrind - the compiler can reorder
   // the return value check with the `outcome` check, b/27651442.
   bool outcome = false;
@@ -658,27 +666,23 @@ void InstructionSimplifierVisitor::VisitCheckCast(HCheckCast* check_cast) {
       MaybeRecordStat(stats_, MethodCompilationStat::kRemovedCheckedCast);
       if (check_cast->GetTypeCheckKind() != TypeCheckKind::kBitstringCheck) {
         HLoadClass* load_class = check_cast->GetTargetClass();
-        if (!load_class->HasUses()) {
+        if (!load_class->HasUses() && !load_class->NeedsAccessCheck()) {
           // We cannot rely on DCE to remove the class because the `HLoadClass` thinks it can throw.
-          // However, here we know that it cannot because the checkcast was successfull, hence
+          // However, here we know that it cannot because the checkcast was successful, hence
           // the class was already loaded.
           load_class->GetBlock()->RemoveInstruction(load_class);
         }
       }
     } else {
-      // Don't do anything for exceptional cases for now. Ideally we should remove
-      // all instructions and blocks this instruction dominates.
+      // TODO Don't do anything for exceptional cases for now. Ideally we should
+      // remove all instructions and blocks this instruction dominates and
+      // replace it with a manual throw.
     }
   }
 }
 
 void InstructionSimplifierVisitor::VisitInstanceOf(HInstanceOf* instruction) {
   HInstruction* object = instruction->InputAt(0);
-  if (instruction->GetTypeCheckKind() != TypeCheckKind::kBitstringCheck &&
-      instruction->GetTargetClass()->NeedsAccessCheck()) {
-    // If we need to perform an access check we cannot remove the instruction.
-    return;
-  }
 
   bool can_be_null = true;
   if (CanEnsureNotNullAt(object, instruction)) {
@@ -695,6 +699,11 @@ void InstructionSimplifierVisitor::VisitInstanceOf(HInstanceOf* instruction) {
     return;
   }
 
+  // Minor correctness check.
+  DCHECK(instruction->GetTargetClass()->StrictlyDominates(instruction))
+      << "Illegal graph!\n"
+      << instruction->DumpWithArgs();
+
   // Historical note: The `outcome` was initialized to please Valgrind - the compiler can reorder
   // the return value check with the `outcome` check, b/27651442.
   bool outcome = false;
@@ -713,10 +722,11 @@ void InstructionSimplifierVisitor::VisitInstanceOf(HInstanceOf* instruction) {
     instruction->GetBlock()->RemoveInstruction(instruction);
     if (outcome && instruction->GetTypeCheckKind() != TypeCheckKind::kBitstringCheck) {
       HLoadClass* load_class = instruction->GetTargetClass();
-      if (!load_class->HasUses()) {
-        // We cannot rely on DCE to remove the class because the `HLoadClass` thinks it can throw.
-        // However, here we know that it cannot because the instanceof check was successfull, hence
-        // the class was already loaded.
+      if (!load_class->HasUses() && !load_class->NeedsAccessCheck()) {
+        // We cannot rely on DCE to remove the class because the `HLoadClass`
+        // thinks it can throw. However, here we know that it cannot because the
+        // instanceof check was successful and we don't need to check the
+        // access, hence the class was already loaded.
         load_class->GetBlock()->RemoveInstruction(load_class);
       }
     }