Revert "Add an environment to the beginning of catch blocks"

This reverts commit f976aa822dd35496e4e936b5802af0d53d39ac95.

Reason for revert: breaking some tests https://ci.chromium.org/ui/p/art/builders/ci/angler-armv8-ndebug/3244/blamelist

Change-Id: I5c2732e81bef8a7e83b661b1b947d7c079994c1b

5 files changed, 62 insertions, 138 deletions

diff --git a/compiler/optimizing/code_generator.cc b/compiler/optimizing/code_generator.cc
index 252e756737..b514f9bf9f 100644
--- a/compiler/optimizing/code_generator.cc
+++ b/compiler/optimizing/code_generator.cc
@@ -413,12 +413,6 @@ void CodeGenerator::Compile(CodeAllocator* allocator) {
     for (HInstructionIterator it(block->GetInstructions()); !it.Done(); it.Advance()) {
       HInstruction* current = it.Current();
       if (current->HasEnvironment()) {
-        // Catch StackMaps are dealt with later on in `RecordCatchBlockInfo`.
-        if (block->IsCatchBlock() && block->GetFirstInstruction() == current) {
-          DCHECK(current->IsNop());
-          continue;
-        }
-
         // Create stackmap for HNop or any instruction which calls native code.
         // Note that we need correct mapping for the native PC of the call instruction,
         // so the runtime's stackmap is not sufficient since it is at PC after the call.
@@ -1340,29 +1334,53 @@ void CodeGenerator::RecordCatchBlockInfo() {
       continue;
     }
 
-    // Get the outer dex_pc
-    uint32_t outer_dex_pc = block->GetDexPc();
-    DCHECK(block->GetFirstInstruction()->IsNop());
-    DCHECK(block->GetFirstInstruction()->AsNop()->NeedsEnvironment());
-    HEnvironment* const environment = block->GetFirstInstruction()->GetEnvironment();
-    DCHECK(environment != nullptr);
-    HEnvironment* outer_environment = environment;
-    while (outer_environment->GetParent() != nullptr) {
-      outer_environment = outer_environment->GetParent();
-    }
-    outer_dex_pc = outer_environment->GetDexPc();
-
+    uint32_t dex_pc = block->GetDexPc();
+    uint32_t num_vregs = graph_->GetNumberOfVRegs();
     uint32_t native_pc = GetAddressOf(block);
-    stack_map_stream->BeginStackMapEntry(outer_dex_pc,
+
+    stack_map_stream->BeginStackMapEntry(dex_pc,
                                          native_pc,
                                          /* register_mask= */ 0,
                                          /* sp_mask= */ nullptr,
                                          StackMap::Kind::Catch);
 
-    EmitEnvironment(environment,
-                    /* slow_path= */ nullptr,
-                    /* needs_vreg_info= */ true,
-                    /* is_for_catch_handler= */ true);
+    HInstruction* current_phi = block->GetFirstPhi();
+    for (size_t vreg = 0; vreg < num_vregs; ++vreg) {
+      while (current_phi != nullptr && current_phi->AsPhi()->GetRegNumber() < vreg) {
+        HInstruction* next_phi = current_phi->GetNext();
+        DCHECK(next_phi == nullptr ||
+               current_phi->AsPhi()->GetRegNumber() <= next_phi->AsPhi()->GetRegNumber())
+            << "Phis need to be sorted by vreg number to keep this a linear-time loop.";
+        current_phi = next_phi;
+      }
+
+      if (current_phi == nullptr || current_phi->AsPhi()->GetRegNumber() != vreg) {
+        stack_map_stream->AddDexRegisterEntry(DexRegisterLocation::Kind::kNone, 0);
+      } else {
+        Location location = current_phi->GetLocations()->Out();
+        switch (location.GetKind()) {
+          case Location::kStackSlot: {
+            stack_map_stream->AddDexRegisterEntry(
+                DexRegisterLocation::Kind::kInStack, location.GetStackIndex());
+            break;
+          }
+          case Location::kDoubleStackSlot: {
+            stack_map_stream->AddDexRegisterEntry(
+                DexRegisterLocation::Kind::kInStack, location.GetStackIndex());
+            stack_map_stream->AddDexRegisterEntry(
+                DexRegisterLocation::Kind::kInStack, location.GetHighStackIndex(kVRegSize));
+            ++vreg;
+            DCHECK_LT(vreg, num_vregs);
+            break;
+          }
+          default: {
+            // All catch phis must be allocated to a stack slot.
+            LOG(FATAL) << "Unexpected kind " << location.GetKind();
+            UNREACHABLE();
+          }
+        }
+      }
+    }
 
     stack_map_stream->EndStackMapEntry();
   }
@@ -1373,9 +1391,7 @@ void CodeGenerator::AddSlowPath(SlowPathCode* slow_path) {
   code_generation_data_->AddSlowPath(slow_path);
 }
 
-void CodeGenerator::EmitVRegInfo(HEnvironment* environment,
-                                 SlowPathCode* slow_path,
-                                 bool is_for_catch_handler) {
+void CodeGenerator::EmitVRegInfo(HEnvironment* environment, SlowPathCode* slow_path) {
   StackMapStream* stack_map_stream = GetStackMapStream();
   // Walk over the environment, and record the location of dex registers.
   for (size_t i = 0, environment_size = environment->Size(); i < environment_size; ++i) {
@@ -1430,7 +1446,6 @@ void CodeGenerator::EmitVRegInfo(HEnvironment* environment,
       }
 
       case Location::kRegister : {
-        DCHECK(!is_for_catch_handler);
         int id = location.reg();
         if (slow_path != nullptr && slow_path->IsCoreRegisterSaved(id)) {
           uint32_t offset = slow_path->GetStackOffsetOfCoreRegister(id);
@@ -1452,7 +1467,6 @@ void CodeGenerator::EmitVRegInfo(HEnvironment* environment,
       }
 
       case Location::kFpuRegister : {
-        DCHECK(!is_for_catch_handler);
         int id = location.reg();
         if (slow_path != nullptr && slow_path->IsFpuRegisterSaved(id)) {
           uint32_t offset = slow_path->GetStackOffsetOfFpuRegister(id);
@@ -1474,7 +1488,6 @@ void CodeGenerator::EmitVRegInfo(HEnvironment* environment,
       }
 
       case Location::kFpuRegisterPair : {
-        DCHECK(!is_for_catch_handler);
         int low = location.low();
         int high = location.high();
         if (slow_path != nullptr && slow_path->IsFpuRegisterSaved(low)) {
@@ -1496,7 +1509,6 @@ void CodeGenerator::EmitVRegInfo(HEnvironment* environment,
       }
 
       case Location::kRegisterPair : {
-        DCHECK(!is_for_catch_handler);
         int low = location.low();
         int high = location.high();
         if (slow_path != nullptr && slow_path->IsCoreRegisterSaved(low)) {
@@ -1527,54 +1539,9 @@ void CodeGenerator::EmitVRegInfo(HEnvironment* environment,
   }
 }
 
-void CodeGenerator::EmitVRegInfoOnlyCatchPhis(HEnvironment* environment) {
-  StackMapStream* stack_map_stream = GetStackMapStream();
-  DCHECK(environment->GetHolder()->GetBlock()->IsCatchBlock());
-  DCHECK_EQ(environment->GetHolder()->GetBlock()->GetFirstInstruction(), environment->GetHolder());
-  HInstruction* current_phi = environment->GetHolder()->GetBlock()->GetFirstPhi();
-  for (size_t vreg = 0; vreg < environment->Size(); ++vreg) {
-    while (current_phi != nullptr && current_phi->AsPhi()->GetRegNumber() < vreg) {
-      HInstruction* next_phi = current_phi->GetNext();
-      DCHECK(next_phi == nullptr ||
-             current_phi->AsPhi()->GetRegNumber() <= next_phi->AsPhi()->GetRegNumber())
-          << "Phis need to be sorted by vreg number to keep this a linear-time loop.";
-      current_phi = next_phi;
-    }
-
-    if (current_phi == nullptr || current_phi->AsPhi()->GetRegNumber() != vreg) {
-      stack_map_stream->AddDexRegisterEntry(DexRegisterLocation::Kind::kNone, 0);
-    } else {
-      Location location = current_phi->GetLocations()->Out();
-      switch (location.GetKind()) {
-        case Location::kStackSlot: {
-          stack_map_stream->AddDexRegisterEntry(DexRegisterLocation::Kind::kInStack,
-                                                location.GetStackIndex());
-          break;
-        }
-        case Location::kDoubleStackSlot: {
-          stack_map_stream->AddDexRegisterEntry(DexRegisterLocation::Kind::kInStack,
-                                                location.GetStackIndex());
-          stack_map_stream->AddDexRegisterEntry(DexRegisterLocation::Kind::kInStack,
-                                                location.GetHighStackIndex(kVRegSize));
-          ++vreg;
-          DCHECK_LT(vreg, environment->Size());
-          break;
-        }
-        default: {
-          LOG(FATAL) << "All catch phis must be allocated to a stack slot. Unexpected kind "
-                     << location.GetKind();
-          UNREACHABLE();
-        }
-      }
-    }
-  }
-}
-
 void CodeGenerator::EmitEnvironment(HEnvironment* environment,
                                     SlowPathCode* slow_path,
-                                    bool needs_vreg_info,
-                                    bool is_for_catch_handler,
-                                    bool innermost_environment) {
+                                    bool needs_vreg_info) {
   if (environment == nullptr) return;
 
   StackMapStream* stack_map_stream = GetStackMapStream();
@@ -1582,11 +1549,7 @@ void CodeGenerator::EmitEnvironment(HEnvironment* environment,
 
   if (emit_inline_info) {
     // We emit the parent environment first.
-    EmitEnvironment(environment->GetParent(),
-                    slow_path,
-                    needs_vreg_info,
-                    is_for_catch_handler,
-                    /* innermost_environment= */ false);
+    EmitEnvironment(environment->GetParent(), slow_path, needs_vreg_info);
     stack_map_stream->BeginInlineInfoEntry(environment->GetMethod(),
                                            environment->GetDexPc(),
                                            needs_vreg_info ? environment->Size() : 0,
@@ -1594,13 +1557,9 @@ void CodeGenerator::EmitEnvironment(HEnvironment* environment,
                                            this);
   }
 
-  // If a dex register map is not required we just won't emit it.
   if (needs_vreg_info) {
-    if (innermost_environment && is_for_catch_handler) {
-      EmitVRegInfoOnlyCatchPhis(environment);
-    } else {
-      EmitVRegInfo(environment, slow_path, is_for_catch_handler);
-    }
+    // If a dex register map is not required we just won't emit it.
+    EmitVRegInfo(environment, slow_path);
   }
 
   if (emit_inline_info) {
diff --git a/compiler/optimizing/code_generator.h b/compiler/optimizing/code_generator.h
index 6b85aaa880..de247a98b9 100644
--- a/compiler/optimizing/code_generator.h
+++ b/compiler/optimizing/code_generator.h
@@ -846,11 +846,8 @@ class CodeGenerator : public DeletableArenaObject<kArenaAllocCodeGenerator> {
   void BlockIfInRegister(Location location, bool is_out = false) const;
   void EmitEnvironment(HEnvironment* environment,
                        SlowPathCode* slow_path,
-                       bool needs_vreg_info = true,
-                       bool is_for_catch_handler = false,
-                       bool innermost_environment = true);
-  void EmitVRegInfo(HEnvironment* environment, SlowPathCode* slow_path, bool is_for_catch_handler);
-  void EmitVRegInfoOnlyCatchPhis(HEnvironment* environment);
+                       bool needs_vreg_info = true);
+  void EmitVRegInfo(HEnvironment* environment, SlowPathCode* slow_path);
 
   static void PrepareCriticalNativeArgumentMoves(
       HInvokeStaticOrDirect* invoke,
diff --git a/compiler/optimizing/code_sinking.cc b/compiler/optimizing/code_sinking.cc
index 930675b401..766bb01978 100644
--- a/compiler/optimizing/code_sinking.cc
+++ b/compiler/optimizing/code_sinking.cc
@@ -271,21 +271,10 @@ static HInstruction* FindIdealPosition(HInstruction* instruction,
     }
   }
   for (const HUseListNode<HEnvironment*>& use : instruction->GetEnvUses()) {
-    HEnvironment* env = use.GetUser();
-    HInstruction* user = env->GetHolder();
+    HInstruction* user = use.GetUser()->GetHolder();
     if (user->GetBlock() == target_block &&
         (insert_pos == nullptr || user->StrictlyDominates(insert_pos))) {
-      if (target_block->IsCatchBlock() && target_block->GetFirstInstruction() == user) {
-        // We can sink the instructions past the environment setting Nop. If we do that, we have to
-        // remove said instruction from the environment. Since we know that we will be sinking the
-        // instruction to this block and there are no more instructions to consider, we can safely
-        // remove it from the environment now.
-        DCHECK(target_block->GetFirstInstruction()->IsNop());
-        env->RemoveAsUserOfInput(use.GetIndex());
-        env->SetRawEnvAt(use.GetIndex(), /*instruction=*/ nullptr);
-      } else {
-        insert_pos = user;
-      }
+      insert_pos = user;
     }
   }
   if (insert_pos == nullptr) {
diff --git a/compiler/optimizing/graph_checker.cc b/compiler/optimizing/graph_checker.cc
index 10711510f2..eda6363dda 100644
--- a/compiler/optimizing/graph_checker.cc
+++ b/compiler/optimizing/graph_checker.cc
@@ -159,24 +159,6 @@ void GraphChecker::VisitBasicBlock(HBasicBlock* block) {
     }
   }
 
-  // Make sure the first instruction of a catch block is always a Nop that emits an environment.
-  if (block->IsCatchBlock()) {
-    if (!block->GetFirstInstruction()->IsNop()) {
-      AddError(StringPrintf("Block %d doesn't have a Nop as its first instruction.",
-                            current_block_->GetBlockId()));
-    } else {
-      HNop* nop = block->GetFirstInstruction()->AsNop();
-      if (!nop->NeedsEnvironment()) {
-        AddError(
-            StringPrintf("%s:%d is a Nop and the first instruction of block %d, but it doesn't "
-                         "need an environment.",
-                         nop->DebugName(),
-                         nop->GetId(),
-                         current_block_->GetBlockId()));
-      }
-    }
-  }
-
   // Visit this block's list of phis.
   for (HInstructionIterator it(block->GetPhis()); !it.Done(); it.Advance()) {
     HInstruction* current = it.Current();
@@ -360,15 +342,14 @@ void GraphChecker::VisitTryBoundary(HTryBoundary* try_boundary) {
 }
 
 void GraphChecker::VisitLoadException(HLoadException* load) {
-  // Ensure that LoadException is the second instruction in a catch block. The first one should be a
-  // Nop (checked separately).
+  // Ensure that LoadException is the first instruction in a catch block.
   if (!load->GetBlock()->IsCatchBlock()) {
     AddError(StringPrintf("%s:%d is in a non-catch block %d.",
                           load->DebugName(),
                           load->GetId(),
                           load->GetBlock()->GetBlockId()));
-  } else if (load->GetBlock()->GetFirstInstruction()->GetNext() != load) {
-    AddError(StringPrintf("%s:%d is not the second instruction in catch block %d.",
+  } else if (load->GetBlock()->GetFirstInstruction() != load) {
+    AddError(StringPrintf("%s:%d is not the first instruction in catch block %d.",
                           load->DebugName(),
                           load->GetId(),
                           load->GetBlock()->GetBlockId()));
@@ -532,10 +513,17 @@ void GraphChecker::VisitInstruction(HInstruction* instruction) {
                           instruction->GetId(),
                           current_block_->GetBlockId()));
   } else if (instruction->CanThrowIntoCatchBlock()) {
-    // Find all catch blocks and test that `instruction` has an environment value for each one.
+    // Find the top-level environment. This corresponds to the environment of
+    // the catch block since we do not inline methods with try/catch.
+    HEnvironment* environment = instruction->GetEnvironment();
+    while (environment->GetParent() != nullptr) {
+      environment = environment->GetParent();
+    }
+
+    // Find all catch blocks and test that `instruction` has an environment
+    // value for each one.
     const HTryBoundary& entry = instruction->GetBlock()->GetTryCatchInformation()->GetTryEntry();
     for (HBasicBlock* catch_block : entry.GetExceptionHandlers()) {
-      const HEnvironment* environment = catch_block->GetFirstInstruction()->GetEnvironment();
       for (HInstructionIterator phi_it(catch_block->GetPhis()); !phi_it.Done(); phi_it.Advance()) {
         HPhi* catch_phi = phi_it.Current()->AsPhi();
         if (environment->GetInstructionAt(catch_phi->GetRegNumber()) == nullptr) {
diff --git a/compiler/optimizing/instruction_builder.cc b/compiler/optimizing/instruction_builder.cc
index ba58c8d1fe..605427ba11 100644
--- a/compiler/optimizing/instruction_builder.cc
+++ b/compiler/optimizing/instruction_builder.cc
@@ -343,10 +343,6 @@ static bool IsBlockPopulated(HBasicBlock* block) {
     // Suspend checks were inserted into loop headers during building of dominator tree.
     DCHECK(block->GetFirstInstruction()->IsSuspendCheck());
     return block->GetFirstInstruction() != block->GetLastInstruction();
-  } else if (block->IsCatchBlock()) {
-    // Nops were inserted into the beginning of catch blocks.
-    DCHECK(block->GetFirstInstruction()->IsNop());
-    return block->GetFirstInstruction() != block->GetLastInstruction();
   } else {
     return !block->GetInstructions().IsEmpty();
   }
@@ -391,11 +387,6 @@ bool HInstructionBuilder::Build() {
       // This is slightly odd because the loop header might not be empty (TryBoundary).
       // But we're still creating the environment with locals from the top of the block.
       InsertInstructionAtTop(suspend_check);
-    } else if (current_block_->IsCatchBlock()) {
-      // We add an environment emitting instruction at the beginning of each catch block, in order
-      // to support try catch inlining.
-      // This is slightly odd because the catch block might not be empty (TryBoundary).
-      InsertInstructionAtTop(new (allocator_) HNop(block_dex_pc, /* needs_environment= */ true));
     }
 
     if (block_dex_pc == kNoDexPc || current_block_ != block_builder_->GetBlockAt(block_dex_pc)) {