MIPS64: Fix 'MIPS: use DAHI/DATH for li macro on mips64r6.'

src/mips64/macro-assembler-mips64.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <limits.h>  // For LONG_MIN, LONG_MAX.
 
 #if V8_TARGET_ARCH_MIPS64
 
 #include "src/base/division-by-constant.h"
 #include "src/bootstrapper.h"
@@ skipping 1360 matching lines @@
         ori(rd, zero_reg, (j.imm64_ & kImm16Mask));
       } else if (!(j.imm64_ & kImm16Mask)) {
         lui(rd, (j.imm64_ >> kLuiShift) & kImm16Mask);
       } else {
         lui(rd, (j.imm64_ >> kLuiShift) & kImm16Mask);
         ori(rd, rd, (j.imm64_ & kImm16Mask));
       }
     } else {
       if (kArchVariant == kMips64r6) {
         int64_t imm = j.imm64_;
-        bool lui_emited = false;
-        if (((imm >> kLuiShift) & kImm16Mask) != 0) {
-          lui(rd, (imm >> kLuiShift) & kImm16Mask);
-          lui_emited = true;
-        }
+        lui(rd, (imm >> kLuiShift) & kImm16Mask);
         if ((imm & kImm16Mask) != 0) {
           ori(rd, rd, (imm & kImm16Mask));
-        } else if (!lui_emited) {
-          or_(rd, zero_reg, zero_reg);
         }

[balazs.kilvady, 2016/02/23 19:56:23]

If I got it correctly at late evening then you use lui; ori for loading the
lower 32 bits of the imm. But if the
if (((imm >> kLuiShift) & kImm16Mask) == 0) then a
daddiu(rd, zero_reg, (j.imm64_ & kImm16Mask));
is quicker/smaller then a lui(rd, 0); ori(rd, rd, j.imm64_ & kImm16Mask);
Why don't you use the upper code block of if (is_int32(j.imm64_)) {} to load the
lower 32 bits? In that case that code could be put in a helper function to keep
the code DRY.

[Alan Li, 2016/02/25 03:05:40]

Done.

         if ((imm >> 31) & 0x1) {

[balazs.kilvady, 2016/02/23 19:56:23]

Don't we have kImm32Mask? If not yet then I think that should be added.

[Alan Li, 2016/02/25 03:05:39]

Consolidated into shiftAndFixSignExtension.

On 2016/02/23 19:56:23, balazs.kilvady wrote:

           imm = (imm >> 32) + 1;
         } else {
           imm = imm >> 32;
         }
         if (imm & kImm16Mask) {
           dahi(rd, imm & kImm16Mask);
         }
         if (!is_int48(j.imm64_)) {
           if ((imm >> 15) & 0x1) {
             imm = (imm >> 16) + 1;
           } else {
             imm = imm >> 16;
           }

[balazs.kilvady, 2016/02/23 19:56:23]

I guess the above if () {} else {} is for fixing sign extension (again). I would
use a static inline function for that:

// In comment: what is it good for.
static inline int64_t fixSignExtension(int64_t imm, int bitnum) {...}
...
imm = fixSignExtension(imm, 32);
...
imm = fixSignExtension(imm, 16);

[Alan Li, 2016/02/25 03:05:40]

Done.

[Alan Li, 2016/02/25 03:05:40]

Done.

           if (imm & kImm16Mask) {
             dati(rd, imm & kImm16Mask);
           }
         }
       } else {
         if (is_int48(j.imm64_)) {
           if ((j.imm64_ >> 32) & kImm16Mask) {
             lui(rd, (j.imm64_ >> 32) & kImm16Mask);
             if ((j.imm64_ >> 16) & kImm16Mask) {
               ori(rd, rd, (j.imm64_ >> 16) & kImm16Mask);
@@ skipping 5246 matching lines @@
   if (mag.shift > 0) sra(result, result, mag.shift);
   srl(at, dividend, 31);
   Addu(result, result, Operand(at));
 }
 
 
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_TARGET_ARCH_MIPS64