[MIPS] Add support to match more patterns for DINS instruction

[spetrovic]

Summary

This patch adds support for recognizing patterns to match DINS instruction. It replaces set of instructions with DINS instruction and one more instruction (add instruction or shift right depending of case).
Example:

#include <stdint.h>
struct buf_str {

struct
{
    unsigned long long addr :37;
    unsigned long long addr1 :15;
    unsigned int length:14;
    uint64_t total_bytes:16;
    uint64_t segs : 6;
} s;

};

void foo(volatile struct buf_str bufptr) {

bufptr.s.segs = 4;

}

Output (objdump) without patch:

Disassembly of section .text:

0000000000000000 <foo>:

 0:	67bdfff0 	daddiu	sp,sp,-16
 4:	ffa50008 	sd	a1,8(sp)
 8:	64010001 	daddiu	at,zero,1
 c:	0001093c 	dsll32	at,at,0x4
10:	6421ffc1 	daddiu	at,at,-63
14:	00010f38 	dsll	at,at,0x1c
18:	6421ffff 	daddiu	at,at,-1
1c:	dfa20008 	ld	v0,8(sp)
20:	00410824 	and	at,v0,at
24:	3c024000 	lui	v0,0x4000
28:	00220825 	or	at,at,v0
2c:	ffa10008 	sd	at,8(sp)
30:	03e00008 	jr	ra
34:	67bd0010 	daddiu	sp,sp,16

Output (objdump) with patch:

Disassembly of section .text:

0000000000000000 <foo>:

 0:	67bdfff0 	daddiu	sp,sp,-16
 4:	ffa50008 	sd	a1,8(sp)
 8:	64010004 	daddiu	at,zero,4
 c:	dfa20008 	ld	v0,8(sp)
10:	7c220f05 	dins	v0,at,0x1c,0x6
14:	ffa20008 	sd	v0,8(sp)
18:	03e00008 	jr	ra
1c:	67bd0010 	daddiu	sp,sp,16

[spetrovic]

ping

[sdardis]

This looks mostly ok, I've highlighted the issues inline. The big change are lines 846 & 847. As it stands, they don't guard against MIPS32 code having out of range values.

lib/Target/Mips/MipsISelLowering.cpp

808 ↗

(On Diff #93376)

Spurious newline.

813–840 ↗

(On Diff #93376)

Correct the indentation for this hunk, as you've introduced a new lexical scope.

845 ↗

(On Diff #93376)

// => dins $dst, $src, pos, size

847 ↗

(On Diff #93376)

This is incorrect for MIPS32. Your provided test case when compiled for MIPS32R2 produces bad assembly + wrong object code. E.g:

ins $1, $2, 18, 46

I believe the conditional needs to be extended along the lines of:

&& ((SMSize0 + SMPos0 <= 64 && Subtarget.hasMips64r2()) || (SMSize0 + SMPos0 <= 32)) {

Also, these two lines need to be clang-formatted.

865–871 ↗

(On Diff #93376)

This hunk needs to be clang-formatted.

test/CodeGen/Mips/dins.ll

1 ↗

(On Diff #93376)

Add testing for mips32r2 for the 'ins' instruction. Also test that out of range values for the ins instruction are not produced.

Also, test that ins / dins are not produced for pre mips32r2 / mips64r2.

3–22 ↗

(On Diff #93376)

This test case can be reduced to:

; struct s {
;   unsigned long long addr :37;
;   unsigned long long addr1 :15;
;   unsigned int length:14;
;   unsigned long long total_bytes:16;
;   unsigned long long segs : 6;
; };

; struct s foo(volatile struct s s1) {
;   s1.addr = 123;
;   s1.segs = 4;
;   s1.length = 5;
;   s1.total_bytes = s1.length;
;   return s1;
; }

The volatile qualifier can be removed, doing so leaves only 2 dins instructions.

23 ↗

(On Diff #93376)

Include a comment stating what you're testing for.

55 ↗

(On Diff #93376)

This needs a CHECK-LABEL:

test/CodeGen/Mips/mips64-f128.ll

428–431 ↗

(On Diff #93376)

Check for dins here for mips64r2.

[spetrovic]

Comments addressed.

[sdardis]

LGTM with some nits addressed.

lib/Target/Mips/MipsISelLowering.cpp

815–834 ↗

(On Diff #94946)

There is some additional whitespace here on the seemingly blank lines.

821–822 ↗

(On Diff #94946)

This check is now redundant.

test/CodeGen/Mips/dins.ll

70 ↗

(On Diff #94946)

You can drop the ${{[0-9]+}} for the MIPS16-NOT. The MIPS16e ISA doesn't contain the insert instruction so we can save a few cycles of testing by just testing for the lack of 'ins'.

[sdardis]

Hi Strahinja,

This caused a regression:

See:

http://lab.llvm.org:8011/builders/clang-cmake-mips/builds/3492

http://lab.llvm.org:8011/builders/clang-cmake-mips/builds/3492/steps/ninja%20check%201/logs/stdio

Thanks,
Simon

[spetrovic]

Hi Simon,

I'm looking into this problem.

Thanks,
Strahinja

[sdardis]

Can you commit a revert while you investigate? I need to investigate some issues with our buildbots.

Thanks,
Simon

[spetrovic]

Hi Simon,

This patch should resolve problem that you reported, can you try this patch on your buildbot to make sure I will not break it again, or I can commit it and see what happens.

Thanks,
Strahinja

[sdardis]

Yes, this seems ok on my end.

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

static SDValue performORCombine(SDNode *N, SelectionDAG &DAG,
   //  $dst = or (and $src1 , mask0), (and (shl $src, pos), mask1),
   //  where mask1 = (2**size - 1) << pos, mask0 = ~mask1
   //  => ins $dst, $src, size, pos, $src1
   if (DCI.isBeforeLegalizeOps() || !Subtarget.hasExtractInsert())
     return SDValue();
 
   SDValue And0 = N->getOperand(0), And1 = N->getOperand(1);
   uint64_t SMPos0, SMSize0, SMPos1, SMSize1;
-  ConstantSDNode *CN;
+  ConstantSDNode *CN, *CN1;
 
   // See if Op's first operand matches (and $src1 , mask0).
   if (And0.getOpcode() != ISD::AND)
     return SDValue();
 
   if (!(CN = dyn_cast<ConstantSDNode>(And0.getOperand(1))) ||
       !isShiftedMask(~CN->getSExtValue(), SMPos0, SMSize0))
     return SDValue();
 
   // See if Op's second operand matches (and (shl $src, pos), mask1).
-  if (And1.getOpcode() != ISD::AND)
-    return SDValue();
+  if (And1.getOpcode() == ISD::AND &&
+      And1.getOperand(0).getOpcode() == ISD::SHL) {
 
     if (!(CN = dyn_cast<ConstantSDNode>(And1.getOperand(1))) ||
         !isShiftedMask(CN->getZExtValue(), SMPos1, SMSize1))
       return SDValue();
 
       // The shift masks must have the same position and size.
       if (SMPos0 != SMPos1 || SMSize0 != SMSize1)
         return SDValue();
 
       SDValue Shl = And1.getOperand(0);
-  if (Shl.getOpcode() != ISD::SHL)
-    return SDValue();
 
       if (!(CN = dyn_cast<ConstantSDNode>(Shl.getOperand(1))))
         return SDValue();
 
       unsigned Shamt = CN->getZExtValue();
 
       // Return if the shift amount and the first bit position of mask are not the
       // same.
       EVT ValTy = N->getValueType(0);
       if ((Shamt != SMPos0) || (SMPos0 + SMSize0 > ValTy.getSizeInBits()))
         return SDValue();
 
       SDLoc DL(N);
       return DAG.getNode(MipsISD::Ins, DL, ValTy, Shl.getOperand(0),
                          DAG.getConstant(SMPos0, DL, MVT::i32),
                          DAG.getConstant(SMSize0, DL, MVT::i32),
                          And0.getOperand(0));
+  } else {
+    // Pattern match DINS.
+    //  $dst = or (and $src, mask0), mask1
+    //  where mask0 = ((1 << SMSize0) -1) << SMPos0
+    //  => dins $dst, $src, pos, size
+    if (~CN->getSExtValue() == ((((int64_t)1 << SMSize0) - 1) << SMPos0) &&
+        ((SMSize0 + SMPos0 <= 64 && Subtarget.hasMips64r2()) ||
+         (SMSize0 + SMPos0 <= 32))) {
+      // Check if AND instruction has constant as argument
+      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();
+      }
+      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);
+      }
+      return DAG.getNode(
+          MipsISD::Ins, DL, N->getValueType(0),
+          isConstCase
+              ? DAG.getConstant(CN1->getSExtValue() >> SMPos0, DL, ValTy)
+              : SrlX,
+          DAG.getConstant(SMPos0, DL, MVT::i32),
+          DAG.getConstant(SMSize0, DL, MVT::i32), And0->getOperand(0));
+
+    }
+    return SDValue();
+  }
 }
 
 static SDValue performADDCombine(SDNode *N, SelectionDAG &DAG,
                                  TargetLowering::DAGCombinerInfo &DCI,
                                  const MipsSubtarget &Subtarget) {
   // (add v0, (add v1, abs_lo(tjt))) => (add (add v0, v1), abs_lo(tjt))
 
   if (DCI.isBeforeLegalizeOps())

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
+
+; #include <stdint.h>
+; #include <stdio.h>
+; struct cvmx_buf_ptr {
+
+;   struct {
+;     unsigned long long addr :37;
+;     unsigned long long addr1 :15;
+;     unsigned int lenght:14;
+;     uint64_t total_bytes:16;
+;     uint64_t segs : 6;
+;   } s;
+; }
+;
+; unsigned long long foo(volatile struct cvmx_buf_ptr bufptr) {
+;   bufptr.s.addr = 123;
+;   bufptr.s.segs = 4;
+;   bufptr.s.lenght = 5;
+;   bufptr.s.total_bytes = bufptr.s.lenght;
+;   return bufptr.s.addr;
+; }
+
+; Testing of selection INS/DINS instruction
+
+define i64 @f123(i64 inreg %bufptr.coerce0, i64 inreg %bufptr.coerce1) local_unnamed_addr #0 {
+entry:
+  %bufptr.sroa.0 = alloca i64, align 8
+  %bufptr.sroa.4 = alloca i64, align 8
+  store i64 %bufptr.coerce0, i64* %bufptr.sroa.0, align 8
+  store i64 %bufptr.coerce1, i64* %bufptr.sroa.4, align 8
+  %bufptr.sroa.0.0.bufptr.sroa.0.0.bufptr.sroa.0.0.bf.load = load volatile i64, i64* %bufptr.sroa.0, align 8
+  %bf.clear = and i64 %bufptr.sroa.0.0.bufptr.sroa.0.0.bufptr.sroa.0.0.bf.load, 134217727
+  %bf.set = or i64 %bf.clear, 16508780544
+  store volatile i64 %bf.set, i64* %bufptr.sroa.0, align 8
+  %bufptr.sroa.4.0.bufptr.sroa.4.0.bufptr.sroa.4.8.bf.load2 = load volatile i64, i64* %bufptr.sroa.4, align 8
+  %bf.clear3 = and i64 %bufptr.sroa.4.0.bufptr.sroa.4.0.bufptr.sroa.4.8.bf.load2, -16911433729
+  %bf.set4 = or i64 %bf.clear3, 1073741824
+  store volatile i64 %bf.set4, i64* %bufptr.sroa.4, align 8
+  %bufptr.sroa.4.0.bufptr.sroa.4.0.bufptr.sroa.4.8.bf.load6 = load volatile i64, i64* %bufptr.sroa.4, align 8
+  %bf.clear7 = and i64 %bufptr.sroa.4.0.bufptr.sroa.4.0.bufptr.sroa.4.8.bf.load6, 1125899906842623
+  %bf.set8 = or i64 %bf.clear7, 5629499534213120
+  store volatile i64 %bf.set8, i64* %bufptr.sroa.4, align 8
+  %bufptr.sroa.4.0.bufptr.sroa.4.0.bufptr.sroa.4.8.bf.load11 = load volatile i64, i64* %bufptr.sroa.4, align 8
+  %bf.lshr = lshr i64 %bufptr.sroa.4.0.bufptr.sroa.4.0.bufptr.sroa.4.8.bf.load11, 50
+  %bufptr.sroa.4.0.bufptr.sroa.4.0.bufptr.sroa.4.8.bf.load13 = load volatile i64, i64* %bufptr.sroa.4, align 8
+  %bf.shl = shl nuw nsw i64 %bf.lshr, 34
+  %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: dins    $[[R0:[0-9]+]], $[[R1:[0-9]+]], 27, 37
+; 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: 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:]].*}}
 No newline at end of file

llvm/trunk/test/CodeGen/Mips/mips64-f128.ll

 entry:
   %0 = tail call fp128 @llvm.powi.f128(fp128 %a, i32 %b)
   ret fp128 %0
 }
 
 declare fp128 @llvm.powi.f128(fp128, i32) #3
 
 ; ALL-LABEL:     libcall2_copysignl:
-; ALL-DAG:      daddiu $[[R2:[0-9]+]], $zero, 1
-; ALL-DAG:      dsll   $[[R3:[0-9]+]], $[[R2]], 63
+; NOT-R2R6-DAG: daddiu $[[R2:[0-9]+]], $zero, 1
+; NOT-R2R6-DAG: dsll   $[[R3:[0-9]+]], $[[R2]], 63
 ; ALL-DAG:      ld     $[[R0:[0-9]+]], %got_disp(gld1)
 ; ALL-DAG:      ld     $[[R1:[0-9]+]], 8($[[R0]])
-; ALL-DAG:      and    $[[R4:[0-9]+]], $[[R1]], $[[R3]]
+; NOT-R2R6-DAG: and    $[[R4:[0-9]+]], $[[R1]], $[[R3]]
 ; ALL-DAG:      ld     $[[R5:[0-9]+]], %got_disp(gld0)
 ; ALL-DAG:      ld     $[[R6:[0-9]+]], 8($[[R5]])
+; R2R6:         dins   $[[R0:[0-9]+]], $[[R1:[0-9]+]], 63, 1
 ; NOT-R2R6-DAG: daddiu $[[R7:[0-9]+]], $[[R3]], -1
 ; NOT-R2R6-DAG: and    $[[R8:[0-9]+]], $[[R6]], $[[R7]]
 ; NOT-R2R6-DAG: or     $4, $[[R8]], $[[R4]]
-; R2R6-DAG:     dextm  $[[R7:[0-9]+]], $[[R6]], 0, 63
-; R2R6-DAG:     or     $4, $[[R7]], $[[R4]]
 ; ALL-DAG:      ld     $2, 0($[[R5]])
 
 define fp128 @libcall2_copysignl() {
 entry:
   %0 = load fp128, fp128* @gld0, align 16
   %1 = load fp128, fp128* @gld1, align 16
   %call = tail call fp128 @copysignl(fp128 %0, fp128 %1) nounwind readnone
   ret fp128 %call