[MIPS] Fix for selecting of DINS/INS instruction

[spetrovic]

Summary

This patch adds one more condition in selection DINS/INS instruction, which fixes MultiSource/Applications/JM/ldecod/ for mips32r2 (and mips64r2 n32 abi).

[sdardis]

Test case?

[sdardis]

This patch needs a better description of what the issue it's fixing is, and how it's fixing it. Further comments inlined.

lib/Target/Mips/MipsISelLowering.cpp

855–856 ↗

(On Diff #101204)

This requires a comment explaining that we don't produce inserts if the constant operand to the 'or' overlaps with the constant operand of the 'and', and why.

(Provided my reasoning about this patch is correct.)

test/CodeGen/Mips/dins.ll

4 ↗

(On Diff #101204)

The parameters here are wrong, it should be -target-abi=n32 if the check prefix is correct. I'm presuming you meant n32 as the testing already covers n64.

Second issue: is there anything specific to testing n32 codegen that separates it from n64 codegen?

80 ↗

(On Diff #101204)

Remove 'local_unnamed_addr #0' from the define here, it's unnecessary.

92–97 ↗

(On Diff #101204)

Can you reformat this to the same formatting as the checks on line 60-68? I.e. all assembly starts on the same column?

[spetrovic]

test/CodeGen/Mips/dins.ll

4 ↗

(On Diff #101204)

I added n32 testing because the ldecod from test-suite was failing only for n32, but not on n64, although, for this test, the same code is generated for n32 and n64.

[spetrovic]

ping

[mstojanovic]

The patch for which this fix was made (https://reviews.llvm.org/D31465) introduced problems in the Chrome V8 build. After applying this fix all relevant tests pass, so it would be useful if it was committed as soon as possible.

[sdardis]

LGTM.

llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp

if (~CN->getSExtValue() == ((((int64_t)1 << SMSize0) - 1) << SMPos0) &&
       bool isConstCase = And1.getOpcode() != ISD::AND;
       if (And1.getOpcode() == ISD::AND) {
         if (!(CN1 = dyn_cast<ConstantSDNode>(And1->getOperand(1))))
           return SDValue();
       } else {
         if (!(CN1 = dyn_cast<ConstantSDNode>(N->getOperand(1))))
           return SDValue();
       }
+      // Don't generate INS if constant OR operand doesn't fit into bits
+      // cleared by constant AND operand.
+      if (CN->getSExtValue() & CN1->getSExtValue())
+        return SDValue();
+
       SDLoc DL(N);
       EVT ValTy = N->getOperand(0)->getValueType(0);
       SDValue Const1;
       SDValue SrlX;
       if (!isConstCase) {
         Const1 = DAG.getConstant(SMPos0, DL, MVT::i32);
         SrlX = DAG.getNode(ISD::SRL, DL, And1->getValueType(0), And1, Const1);
       }

llvm/trunk/test/CodeGen/Mips/dins.ll

 ; RUN: llc -O2 -march=mips64 -mcpu=mips64r2 -target-abi=n64 < %s -o - | FileCheck %s -check-prefix=MIPS64R2
 ; RUN: llc -O2 -march=mips -mcpu=mips32r2 < %s -o - | FileCheck %s -check-prefix=MIPS32R2
 ; RUN: llc -O2 -march=mips -mattr=mips16 < %s -o - | FileCheck %s -check-prefix=MIPS16
+; RUN: llc -O2 -march=mips64 -mcpu=mips64r2 -target-abi=n32 < %s -o - | FileCheck %s -check-prefix=MIPS64R2N32
 
 ; #include <stdint.h>
 ; #include <stdio.h>
 ; struct cvmx_buf_ptr {
 
 ;   struct {
 ;     unsigned long long addr :37;
 ;     unsigned long long addr1 :15;
entry:
   %bf.clear14 = and i64 %bufptr.sroa.4.0.bufptr.sroa.4.0.bufptr.sroa.4.8.bf.load13, -1125882726973441
   %bf.set15 = or i64 %bf.clear14, %bf.shl
   store volatile i64 %bf.set15, i64* %bufptr.sroa.4, align 8
   %bufptr.sroa.0.0.bufptr.sroa.0.0.bufptr.sroa.0.0.bf.load17 = load volatile i64, i64* %bufptr.sroa.0, align 8
   %bf.lshr18 = lshr i64 %bufptr.sroa.0.0.bufptr.sroa.0.0.bufptr.sroa.0.0.bf.load17, 27
   ret i64 %bf.lshr18
 }
 
-
 ; CHECK-LABEL: f123:
-; MIPS64R2: daddiu  $[[R0:[0-9]+]], $zero, 123
+; MIPS64R2: daddiu  $[[R0:[0-9]+]], $zero, 123
 ; MIPS64R2: dins    $[[R0:[0-9]+]], $[[R1:[0-9]+]], 27, 37
-; MIPS64R2: daddiu  $[[R0:[0-9]+]], $zero, 5
-; MIPS64R2: daddiu  $[[R0:[0-9]+]], $zero, 4
+; MIPS64R2: daddiu  $[[R0:[0-9]+]], $zero, 5
+; MIPS64R2: daddiu  $[[R0:[0-9]+]], $zero, 4
 ; MIPS64R2: dins    $[[R0:[0-9]+]], $[[R1:[0-9]+]], 28, 6
 ; MIPS64R2: dins    $[[R0:[0-9]+]], $[[R1:[0-9]+]], 50, 14
-; MIPS64R2: dsrl    $[[R0:[0-9]+]], $[[R1:[0-9]+]], 50
+; MIPS64R2: dsrl    $[[R0:[0-9]+]], $[[R1:[0-9]+]], 50
 ; MIPS64R2: dins    $[[R0:[0-9]+]], $[[R1:[0-9]+]], 34, 16
 ; MIPS32R2: ins     $[[R0:[0-9]+]], $[[R1:[0-9]+]], 2, 16
 ; MIPS32R2-NOT: ins $[[R0:[0-9]+]], $[[R1:[0-9]+]], 18, 46
-; MIPS16-NOT: ins{{[[:space:]].*}}
+; MIPS16-NOT: ins{{[[:space:]].*}}
+
+
+; int foo(volatile int x) {
+;   int y = x;
+;   y = y & -4;
+;   x = y | 8;
+;   return y;
+; }
+
+define i32 @foo(i32 signext %x) {
+entry:
+  %x.addr = alloca i32, align 4
+  store volatile i32 %x, i32* %x.addr, align 4
+  %x.addr.0.x.addr.0. = load volatile i32, i32* %x.addr, align 4
+  %and = and i32 %x.addr.0.x.addr.0., -4
+  %or = or i32 %and, 8
+  store volatile i32 %or, i32* %x.addr, align 4
+  ret i32 %and
+}
+
+; CHECK-LABEL: foo:
+; MIPS64R2:        ori  $[[R0:[0-9]+]], $[[R0:[0-9]+]], 8
+; MIPS64R2-NOT:    ins {{[[:space:]].*}}
+; MIPS32R2:        ori  $[[R0:[0-9]+]], $[[R0:[0-9]+]], 8
+; MIPS32R2-NOT:    ins {{[[:space:]].*}}
+; MIPS64R2N32:     ori  $[[R0:[0-9]+]], $[[R0:[0-9]+]], 8
+; MIPS64R2N32-NOT: ins {{[[:space:]].*}}
 No newline at end of file