OSDN Git Service

[optimizing] Address x86_64 RIP patch comments
authorMark Mendell <mark.p.mendell@intel.com>
Fri, 10 Apr 2015 00:42:42 +0000 (20:42 -0400)
committerMark Mendell <mark.p.mendell@intel.com>
Fri, 10 Apr 2015 12:50:40 +0000 (08:50 -0400)
Nicolas had some comments after the patch
https://android-review.googlesource.com/#/c/144100 had merged.  Fix the
problems that he found.

Change-Id: I40e8a4273997860db7511dc8f1986281b72bead2
Signed-off-by: Mark Mendell <mark.p.mendell@intel.com>
compiler/optimizing/code_generator_x86_64.cc
compiler/optimizing/code_generator_x86_64.h
compiler/optimizing/intrinsics_x86_64.cc
compiler/utils/x86_64/assembler_x86_64.cc
compiler/utils/x86_64/assembler_x86_64.h

index c915b0f..5710ec5 100644 (file)
@@ -4234,13 +4234,14 @@ void InstructionCodeGeneratorX86_64::VisitBoundType(HBoundType* instruction) {
 
 void CodeGeneratorX86_64::Finalize(CodeAllocator* allocator) {
   // Generate the constant area if needed.
-  if (!__ IsConstantAreaEmpty()) {
+  X86_64Assembler* assembler = GetAssembler();
+  if (!assembler->IsConstantAreaEmpty()) {
     // Align to 4 byte boundary to reduce cache misses, as the data is 4 and 8
     // byte values.  If used for vectors at a later time, this will need to be
     // updated to 16 bytes with the appropriate offset.
-    __ Align(4, 0);
-    constant_area_start_ = __ CodeSize();
-    __ AddConstantArea();
+    assembler->Align(4, 0);
+    constant_area_start_ = assembler->CodeSize();
+    assembler->AddConstantArea();
   }
 
   // And finish up.
@@ -4252,7 +4253,7 @@ void CodeGeneratorX86_64::Finalize(CodeAllocator* allocator) {
  */
 class RIPFixup : public AssemblerFixup, public ArenaObject<kArenaAllocMisc> {
   public:
-    RIPFixup(CodeGeneratorX86_64& codegen, int offset)
+    RIPFixup(const CodeGeneratorX86_64& codegen, int offset)
       : codegen_(codegen), offset_into_constant_area_(offset) {}
 
   private:
@@ -4266,7 +4267,7 @@ class RIPFixup : public AssemblerFixup, public ArenaObject<kArenaAllocMisc> {
       region.StoreUnaligned<int32_t>(pos - 4, relative_position);
     }
 
-    CodeGeneratorX86_64& codegen_;
+    const CodeGeneratorX86_64& codegen_;
 
     // Location in constant area that the fixup refers to.
     int offset_into_constant_area_;
index c819eec..aae7de0 100644 (file)
@@ -297,7 +297,7 @@ class CodeGeneratorX86_64 : public CodeGenerator {
   X86_64Assembler assembler_;
   const X86_64InstructionSetFeatures& isa_features_;
 
-  // Offset to start of the constant area in the assembled code.
+  // Offset to the start of the constant area in the assembled code.
   // Used for fixups to the constant area.
   int constant_area_start_;
 
index c0c4ff3..cbf94f0 100644 (file)
@@ -301,15 +301,19 @@ static void CreateFloatToFloatPlusTemps(ArenaAllocator* arena, HInvoke* invoke)
   locations->AddTemp(Location::RequiresFpuRegister());  // FP reg to hold mask.
 }
 
-static void MathAbsFP(LocationSummary* locations, bool is64bit,
-                      X86_64Assembler* assembler, CodeGeneratorX86_64* codegen) {
+static void MathAbsFP(LocationSummary* locations,
+                      bool is64bit,
+                      X86_64Assembler* assembler,
+                      CodeGeneratorX86_64* codegen) {
   Location output = locations->Out();
 
   if (output.IsFpuRegister()) {
     // In-register
     XmmRegister xmm_temp = locations->GetTemp(0).AsFpuRegister<XmmRegister>();
 
-    // TODO: Can mask directly with constant area if we align on 16 bytes.
+    // TODO: Can mask directly with constant area using pand if we can guarantee
+    // that the literal is aligned on a 16 byte boundary.  This will avoid a
+    // temporary.
     if (is64bit) {
       __ movsd(xmm_temp, codegen->LiteralInt64Address(INT64_C(0x7FFFFFFFFFFFFFFF)));
       __ andpd(output.AsFpuRegister<XmmRegister>(), xmm_temp);
@@ -397,8 +401,11 @@ void IntrinsicCodeGeneratorX86_64::VisitMathAbsLong(HInvoke* invoke) {
   GenAbsInteger(invoke->GetLocations(), true, GetAssembler());
 }
 
-static void GenMinMaxFP(LocationSummary* locations, bool is_min, bool is_double,
-                        X86_64Assembler* assembler, CodeGeneratorX86_64* codegen) {
+static void GenMinMaxFP(LocationSummary* locations,
+                        bool is_min,
+                        bool is_double,
+                        X86_64Assembler* assembler,
+                        CodeGeneratorX86_64* codegen) {
   Location op1_loc = locations->InAt(0);
   Location op2_loc = locations->InAt(1);
   Location out_loc = locations->Out();
index cb6d400..638659d 100644 (file)
@@ -2742,14 +2742,14 @@ void X86_64ExceptionSlowPath::Emit(Assembler *sasm) {
 
 void X86_64Assembler::AddConstantArea() {
   const std::vector<int32_t>& area = constant_area_.GetBuffer();
-  for (size_t i = 0, u = area.size(); i < u; i++) {
+  for (size_t i = 0, e = area.size(); i < e; i++) {
     AssemblerBuffer::EnsureCapacity ensured(&buffer_);
     EmitInt32(area[i]);
   }
 }
 
 int ConstantArea::AddInt32(int32_t v) {
-  for (size_t i = 0, u = buffer_.size(); i < u; i++) {
+  for (size_t i = 0, e = buffer_.size(); i < e; i++) {
     if (v == buffer_[i]) {
       return i * elem_size_;
     }
@@ -2766,8 +2766,8 @@ int ConstantArea::AddInt64(int64_t v) {
   int32_t v_high = v >> 32;
   if (buffer_.size() > 1) {
     // Ensure we don't pass the end of the buffer.
-    for (size_t i = 0, u = buffer_.size() - 1; i < u; i++) {
-      if (v_low == buffer_[i] && v_high == buffer_[i+1]) {
+    for (size_t i = 0, e = buffer_.size() - 1; i < e; i++) {
+      if (v_low == buffer_[i] && v_high == buffer_[i + 1]) {
         return i * elem_size_;
       }
     }
index ef6205c..15b8b15 100644 (file)
@@ -235,6 +235,8 @@ class Address : public Operand {
       result.SetSIB(TIMES_1, CpuRegister(RSP), CpuRegister(RBP));
       result.SetDisp32(addr);
     } else {
+      // RIP addressing is done using RBP as the base register.
+      // The value in RBP isn't used.  Instead the offset is added to RIP.
       result.SetModRM(0, CpuRegister(RBP));
       result.SetDisp32(addr);
     }
@@ -244,6 +246,8 @@ class Address : public Operand {
   // An RIP relative address that will be fixed up later.
   static Address RIP(AssemblerFixup* fixup) {
     Address result;
+    // RIP addressing is done using RBP as the base register.
+    // The value in RBP isn't used.  Instead the offset is added to RIP.
     result.SetModRM(0, CpuRegister(RBP));
     result.SetDisp32(0);
     result.SetFixup(fixup);
@@ -267,32 +271,20 @@ class ConstantArea {
   public:
     ConstantArea() {}
 
-    /**
-     * Add a double to the constant area.
-     * @param v literal to be added to the constant area.
-     * @returns the offset in the constant area where the literal resides.
-     */
+    // Add a double to the constant area, returning the offset into
+    // the constant area where the literal resides.
     int AddDouble(double v);
 
-    /**
-     * Add a float to the constant area.
-     * @param v literal to be added to the constant area.
-     * @returns the offset in the constant area where the literal resides.
-     */
+    // Add a float to the constant area, returning the offset into
+    // the constant area where the literal resides.
     int AddFloat(float v);
 
-    /**
-     * Add an int32_t to the constant area.
-     * @param v literal to be added to the constant area.
-     * @returns the offset in the constant area where the literal resides.
-     */
+    // Add an int32_t to the constant area, returning the offset into
+    // the constant area where the literal resides.
     int AddInt32(int32_t v);
 
-    /**
-     * Add an int64_t to the constant area.
-     * @param v literal to be added to the constant area.
-     * @returns the offset in the constant area where the literal resides.
-     */
+    // Add an int64_t to the constant area, returning the offset into
+    // the constant area where the literal resides.
     int AddInt64(int64_t v);
 
     int GetSize() const {
@@ -736,43 +728,26 @@ class X86_64Assembler FINAL : public Assembler {
   // and branch to a ExceptionSlowPath if it is.
   void ExceptionPoll(ManagedRegister scratch, size_t stack_adjust) OVERRIDE;
 
-  /**
-   * Add a double to the constant area.
-   * @param v literal to be added to the constant area.
-   * @returns the offset in the constant area where the literal resides.
-   */
+  // Add a double to the constant area, returning the offset into
+  // the constant area where the literal resides.
   int AddDouble(double v) { return constant_area_.AddDouble(v); }
 
-  /**
-   * Add a float to the constant area.
-   * @param v literal to be added to the constant area.
-   * @returns the offset in the constant area where the literal resides.
-   */
+  // Add a float to the constant area, returning the offset into
+  // the constant area where the literal resides.
   int AddFloat(float v)   { return constant_area_.AddFloat(v); }
 
-  /**
-   * Add an int32_t to the constant area.
-   * @param v literal to be added to the constant area.
-   * @returns the offset in the constant area where the literal resides.
-   */
+  // Add an int32_t to the constant area, returning the offset into
+  // the constant area where the literal resides.
   int AddInt32(int32_t v) { return constant_area_.AddInt32(v); }
 
-  /**
-   * Add an int64_t to the constant area.
-   * @param v literal to be added to the constant area.
-   * @returns the offset in the constant area where the literal resides.
-   */
+  // Add an int64_t to the constant area, returning the offset into
+  // the constant area where the literal resides.
   int AddInt64(int64_t v) { return constant_area_.AddInt64(v); }
 
-  /**
-   * Add the contents of the constant area to the assembler buffer.
-   */
+  // Add the contents of the constant area to the assembler buffer.
   void AddConstantArea();
 
-  /**
-   * Is the constant area empty?
-   * @returns 'true' if there are no literals in the constant area.
-   */
+  // Is the constant area empty? Return true if there are no literals in the constant area.
   bool IsConstantAreaEmpty() const { return constant_area_.GetSize() == 0; }
 
  private: