Merge "MIPS32: Ensure preservation of RA in leaf methods if it's clobbered"

4 files changed, 76 insertions, 7 deletions

diff --git a/compiler/optimizing/code_generator.h b/compiler/optimizing/code_generator.h
index 00a779ad95..0c60a98139 100644
--- a/compiler/optimizing/code_generator.h
+++ b/compiler/optimizing/code_generator.h
@@ -624,7 +624,7 @@ class CodeGenerator : public DeletableArenaObject<kArenaAllocCodeGenerator> {
     return POPCOUNT(core_spill_mask_) * GetWordSize();
   }
 
-  bool HasAllocatedCalleeSaveRegisters() const {
+  virtual bool HasAllocatedCalleeSaveRegisters() const {
     // We check the core registers against 1 because it always comprises the return PC.
     return (POPCOUNT(allocated_registers_.GetCoreRegisters() & core_callee_save_mask_) != 1)
       || (POPCOUNT(allocated_registers_.GetFloatingPointRegisters() & fpu_callee_save_mask_) != 0);
diff --git a/compiler/optimizing/code_generator_mips.cc b/compiler/optimizing/code_generator_mips.cc
index b0737751a7..36bb55ab12 100644
--- a/compiler/optimizing/code_generator_mips.cc
+++ b/compiler/optimizing/code_generator_mips.cc
@@ -666,16 +666,17 @@ void CodeGeneratorMIPS::ComputeSpillMask() {
   if ((fpu_spill_mask_ != 0) && (POPCOUNT(core_spill_mask_) % 2 != 0)) {
     core_spill_mask_ |= (1 << ZERO);
   }
+}
+
+bool CodeGeneratorMIPS::HasAllocatedCalleeSaveRegisters() const {
   // If RA is clobbered by PC-relative operations on R2 and it's the only spilled register
-  // (this can happen in leaf methods), artificially spill the ZERO register in order to
-  // force explicit saving and restoring of RA. RA isn't saved/restored when it's the only
-  // spilled register.
+  // (this can happen in leaf methods), force CodeGenerator::InitializeCodeGeneration()
+  // into the path that creates a stack frame so that RA can be explicitly saved and restored.
+  // RA can't otherwise be saved/restored when it's the only spilled register.
   // TODO: Can this be improved? It causes creation of a stack frame (while RA might be
   // saved in an unused temporary register) and saving of RA and the current method pointer
   // in the frame.
-  if (clobbered_ra_ && core_spill_mask_ == (1u << RA) && fpu_spill_mask_ == 0) {
-    core_spill_mask_ |= (1 << ZERO);
-  }
+  return CodeGenerator::HasAllocatedCalleeSaveRegisters() || clobbered_ra_;
 }
 
 static dwarf::Reg DWARFReg(Register reg) {
@@ -698,6 +699,9 @@ void CodeGeneratorMIPS::GenerateFrameEntry() {
   }
 
   if (HasEmptyFrame()) {
+    CHECK_EQ(fpu_spill_mask_, 0u);
+    CHECK_EQ(core_spill_mask_, 1u << RA);
+    CHECK(!clobbered_ra_);
     return;
   }
 
@@ -731,6 +735,7 @@ void CodeGeneratorMIPS::GenerateFrameEntry() {
   }
 
   // Store the current method pointer.
+  // TODO: can we not do this if RequiresCurrentMethod() returns false?
   __ StoreToOffset(kStoreWord, kMethodRegisterArgument, SP, kCurrentMethodStackOffset);
 }
 
diff --git a/compiler/optimizing/code_generator_mips.h b/compiler/optimizing/code_generator_mips.h
index 4ce54b6097..7ba6c0da0c 100644
--- a/compiler/optimizing/code_generator_mips.h
+++ b/compiler/optimizing/code_generator_mips.h
@@ -274,6 +274,7 @@ class CodeGeneratorMIPS : public CodeGenerator {
   virtual ~CodeGeneratorMIPS() {}
 
   void ComputeSpillMask() OVERRIDE;
+  bool HasAllocatedCalleeSaveRegisters() const OVERRIDE;
   void GenerateFrameEntry() OVERRIDE;
   void GenerateFrameExit() OVERRIDE;
 
diff --git a/compiler/optimizing/codegen_test.cc b/compiler/optimizing/codegen_test.cc
index fe6c0a305e..d00a7867e8 100644
--- a/compiler/optimizing/codegen_test.cc
+++ b/compiler/optimizing/codegen_test.cc
@@ -991,4 +991,67 @@ TEST_F(CodegenTest, ComparisonsLong) {
   }
 }
 
+#ifdef ART_ENABLE_CODEGEN_mips
+TEST_F(CodegenTest, MipsClobberRA) {
+  std::unique_ptr<const MipsInstructionSetFeatures> features_mips(
+      MipsInstructionSetFeatures::FromCppDefines());
+  if (!CanExecute(kMips) || features_mips->IsR6()) {
+    // HMipsComputeBaseMethodAddress and the NAL instruction behind it
+    // should only be generated on non-R6.
+    return;
+  }
+
+  ArenaPool pool;
+  ArenaAllocator allocator(&pool);
+  HGraph* graph = CreateGraph(&allocator);
+
+  HBasicBlock* entry_block = new (&allocator) HBasicBlock(graph);
+  graph->AddBlock(entry_block);
+  graph->SetEntryBlock(entry_block);
+  entry_block->AddInstruction(new (&allocator) HGoto());
+
+  HBasicBlock* block = new (&allocator) HBasicBlock(graph);
+  graph->AddBlock(block);
+
+  HBasicBlock* exit_block = new (&allocator) HBasicBlock(graph);
+  graph->AddBlock(exit_block);
+  graph->SetExitBlock(exit_block);
+  exit_block->AddInstruction(new (&allocator) HExit());
+
+  entry_block->AddSuccessor(block);
+  block->AddSuccessor(exit_block);
+
+  // To simplify matters, don't create PC-relative HLoadClass or HLoadString.
+  // Instead, generate HMipsComputeBaseMethodAddress directly.
+  HMipsComputeBaseMethodAddress* base = new (&allocator) HMipsComputeBaseMethodAddress();
+  block->AddInstruction(base);
+  // HMipsComputeBaseMethodAddress is defined as int, so just make the
+  // compiled method return it.
+  block->AddInstruction(new (&allocator) HReturn(base));
+
+  graph->BuildDominatorTree();
+
+  mips::CodeGeneratorMIPS codegenMIPS(graph, *features_mips.get(), CompilerOptions());
+  // Since there isn't HLoadClass or HLoadString, we need to manually indicate
+  // that RA is clobbered and the method entry code should generate a stack frame
+  // and preserve RA in it. And this is what we're testing here.
+  codegenMIPS.ClobberRA();
+  // Without ClobberRA() the code would be:
+  //   nal              # Sets RA to point to the jr instruction below
+  //   move  v0, ra     # and the CPU falls into an infinite loop.
+  //   jr    ra
+  //   nop
+  // The expected code is:
+  //   addiu sp, sp, -16
+  //   sw    ra, 12(sp)
+  //   sw    a0, 0(sp)
+  //   nal              # Sets RA to point to the lw instruction below.
+  //   move  v0, ra
+  //   lw    ra, 12(sp)
+  //   jr    ra
+  //   addiu sp, sp, 16
+  RunCode(&codegenMIPS, graph, [](HGraph*) {}, false, 0);
+}
+#endif
+
 }  // namespace art