[Peephole] rewrite INSERT_SUBREG to SUBREG_TO_REG if upper bits zero

[Allen]

Summary

Restrict the 32-bit form of an instruction of integer as too many test cases
will be clobber as the register number updated.

From %reg = INSERT_SUBREG %reg, %subreg, subidx
To   %reg:subidx =  SUBREG_TO_REG 0, %subreg, subidx

Try to prefix the redundant mov instruction at D132325 as the SUBREG_TO_REG should not generate code.

[foad]

Nack. Whatever you are trying to do, I think this is the wrong way to do it.

rewrite INSERT_SUBREG to SUBREG_TO_REG if upper bits zero

Where do you actually check that the upper bits are zero?

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

1829–1831

This might be true for some instructions on some targets, but it is not true for Machine IR in general.

1840–1841

I don't think it's acceptable to restrict this to specific types.

[Allen]

In D132939#3758406, @foad wrote:

Where do you actually check that the upper bits are zero?

hi @foad , thanks for you attention, maybe I have some misunderstand. I try this refer to AArch64MIPeepholeOpt::visitORR in file AArch64MIPeepholeOpt.cpp.

[efriedma]

This transform should be in target-specific code so we can ensure the safety. (Or it could use a target hook if it really needs to be in TwoAddressInstructionPass for some reason.)

[Allen]

move this transform in target-specific code to ensure the safety

[Allen]

In D132939#3759126, @efriedma wrote:

This transform should be in target-specific code so we can ensure the safety. (Or it could use a target hook if it really needs to be in TwoAddressInstructionPass for some reason.)

Thanks @efriedma very much for your comment. And I move this transform in the peephole (target-specific code).

[Allen]

ping. Any further concerns? Thanks.

[efriedma]

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

261 ↗

(On Diff #457780)

If I'm understanding correctly, we're assuming the first operand to INSERT_SUBREG is irrelevant because a COPY would destroy the upper part of the register anyway? Maybe makes sense to explicitly note that.

281 ↗

(On Diff #457780)

Checking "!SrcMI" twice.

282 ↗

(On Diff #457780)

TargetOpcode::COPY <= TargetOpcode::GENERIC_OP_END should be true.

284 ↗

(On Diff #457780)

What are you trying to accomplish with these isTypeLegalForClass checks? You're looking for a register class that only contains integer registers? Can you just check that the register class is a subclass of GPR64all?

[Allen]

Address the comment

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

261 ↗

(On Diff #457780)

Ok, I add your comment., thanks

281 ↗

(On Diff #457780)

Thanks, delete the duplicate checking "!SrcMI"

282 ↗

(On Diff #457780)

Thanks, apply your comment

284 ↗

(On Diff #457780)

I add the isTypeLegalForClass checks because this changes will cause many test cases to be updated, especially some vector type register class.

def FPR64 : RegisterClass<"AArch64", [f64, i64, v2f32, v1f64, v8i8, v4i16, v2i32,
                                      v1i64, v4f16, v4bf16],
                                     64, (sequence "D%u", 0, 31)>;

Now I updated with AArch64::GPR64allRegClass.hasSubClassEq(RC) as your comment to check integer registers, thanks!

[efriedma]

LGTM

[MaskRay]

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

295 ↗

(On Diff #458934)

SubregMI caused a -Wunused-variable in -DLLVM_ENABLE_ASSERTIONS=off builds. I fixed it

[Allen]

llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp

295 ↗

(On Diff #458934)

oh, yes, thanks vey much, I see your commit f9b59249757

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

for (MachineBasicBlock::iterator mi = MBB->begin(), me = MBB->end();
       // Now iterate over the information collected above.
       for (auto &TO : TiedOperands) {
         processTiedPairs(&*mi, TO.second, Dist);
         LLVM_DEBUG(dbgs() << "\t\trewrite to:\t" << *mi);
       }
 
       // Rewrite INSERT_SUBREG as COPY now that we no longer need SSA form.
       if (mi->isInsertSubreg()) {
+        Register DstReg = mi->getOperand(0).getReg();
+        const TargetRegisterClass *RC = MRI->getRegClass(DstReg);
+        MachineInstr *SrcMI = MRI->getUniqueVRegDef(mi->getOperand(2).getReg());
+
+        // When you use the 32-bit form of an instruction, the upper 32 bits of
+        // the source registers are ignored and the upper 32 bits of the
+        // destination register are set to zero.
+        //
+        // If Target's 32-bit form of instruction defines the source operand of
+        // zero-extend, we do not need the zero-extend. Let's check the MI's
+        // opcode is real instruction and if it is not, do not process
+        // the opcode conservatively.
+        // Check of isTypeLegalForClass to avoid update too many test cases.
+        if (!SrcMI || (SrcMI->getOpcode() == TargetOpcode::COPY) ||
+            (SrcMI->getOpcode() <= TargetOpcode::GENERIC_OP_END) ||
+            !TRI->isTypeLegalForClass(*RC, MVT::i64) ||
+            TRI->isTypeLegalForClass(*RC, MVT::f64)) {
           // From %reg = INSERT_SUBREG %reg, %subreg, subidx
           // To   %reg:subidx = COPY %subreg
           unsigned SubIdx = mi->getOperand(3).getImm();
           mi->removeOperand(3);
           assert(mi->getOperand(0).getSubReg() == 0 && "Unexpected subreg idx");
           mi->getOperand(0).setSubReg(SubIdx);
           mi->getOperand(0).setIsUndef(mi->getOperand(1).isUndef());
           mi->removeOperand(1);
           mi->setDesc(TII->get(TargetOpcode::COPY));
+        } else {
+          // From %reg = INSERT_SUBREG %reg, %subreg, subidx
+          // To   %reg = SUBREG_TO_REG 0, %subreg, subidx
+          // Build a SUBREG_TO_REG instruction
+          MachineInstr *SubregMI =
+              BuildMI(*mi->getParent(), *mi, mi->getDebugLoc(),
+                      TII->get(TargetOpcode::SUBREG_TO_REG), DstReg)
+                  .addImm(0)
+                  .add(mi->getOperand(2))
+                  .add(mi->getOperand(3));
+          mi->eraseFromParent();
+          mi = SubregMI;
+        }
+
         LLVM_DEBUG(dbgs() << "\t\tconvert to:\t" << *mi);
 
         // Update LiveIntervals.
         if (LIS) {
           Register Reg = mi->getOperand(0).getReg();
           LiveInterval &LI = LIS->getInterval(Reg);
           if (LI.hasSubRanges()) {
             // The COPY no longer defines subregs of %reg except for

llvm/test/CodeGen/AArch64/atomicrmw-O0.ll

 ; NOLSE-NEXT:    str x8, [sp, #24] // 8-byte Folded Spill
 ; NOLSE-NEXT:    b .LBB8_1
 ; NOLSE-NEXT:  .LBB8_1: // %atomicrmw.start
 ; NOLSE-NEXT:    // =>This Loop Header: Depth=1
 ; NOLSE-NEXT:    // Child Loop BB8_2 Depth 2
 ; NOLSE-NEXT:    ldr x9, [sp, #24] // 8-byte Folded Reload
 ; NOLSE-NEXT:    ldr x11, [sp, #16] // 8-byte Folded Reload
 ; NOLSE-NEXT:    mov w8, w9
-; NOLSE-NEXT:    mvn w10, w8
-; NOLSE-NEXT:    // implicit-def: $x8
-; NOLSE-NEXT:    mov w8, w10
+; NOLSE-NEXT:    mvn w8, w8
+; NOLSE-NEXT:    // implicit-def: $x10
+; NOLSE-NEXT:    // kill: def $x8 killed $w8
 ; NOLSE-NEXT:    orr x12, x8, #0xfffffffffffffffe
 ; NOLSE-NEXT:  .LBB8_2: // %atomicrmw.start
 ; NOLSE-NEXT:    // Parent Loop BB8_1 Depth=1
 ; NOLSE-NEXT:    // => This Inner Loop Header: Depth=2
 ; NOLSE-NEXT:    ldaxr x8, [x11]
 ; NOLSE-NEXT:    cmp x8, x9
 ; NOLSE-NEXT:    b.ne .LBB8_4
 ; NOLSE-NEXT:  // %bb.3: // %atomicrmw.start

 ; LSE-NEXT:    ldr x8, [x0]
 ; LSE-NEXT:    str x8, [sp, #24] // 8-byte Folded Spill
 ; LSE-NEXT:    b .LBB8_1
 ; LSE-NEXT:  .LBB8_1: // %atomicrmw.start
 ; LSE-NEXT:    // =>This Inner Loop Header: Depth=1
 ; LSE-NEXT:    ldr x9, [sp, #24] // 8-byte Folded Reload
 ; LSE-NEXT:    ldr x11, [sp, #16] // 8-byte Folded Reload
 ; LSE-NEXT:    mov w8, w9
-; LSE-NEXT:    mvn w10, w8
-; LSE-NEXT:    // implicit-def: $x8
-; LSE-NEXT:    mov w8, w10
+; LSE-NEXT:    mvn w8, w8
+; LSE-NEXT:    // implicit-def: $x10
+; LSE-NEXT:    // kill: def $x8 killed $w8
 ; LSE-NEXT:    orr x10, x8, #0xfffffffffffffffe
 ; LSE-NEXT:    mov x8, x9
 ; LSE-NEXT:    casal x8, x10, [x11]
 ; LSE-NEXT:    subs x9, x8, x9
 ; LSE-NEXT:    cset w9, eq
 ; LSE-NEXT:    str x8, [sp, #8] // 8-byte Folded Spill
 ; LSE-NEXT:    subs w9, w9, #1
 ; LSE-NEXT:    str x8, [sp, #24] // 8-byte Folded Spill

 ; NOLSE-NEXT:    b .LBB9_1
 ; NOLSE-NEXT:  .LBB9_1: // %atomicrmw.start
 ; NOLSE-NEXT:    // =>This Loop Header: Depth=1
 ; NOLSE-NEXT:    // Child Loop BB9_2 Depth 2
 ; NOLSE-NEXT:    ldr x11, [sp, #40] // 8-byte Folded Reload
 ; NOLSE-NEXT:    ldr x8, [sp, #32] // 8-byte Folded Reload
 ; NOLSE-NEXT:    ldr x13, [sp, #24] // 8-byte Folded Reload
 ; NOLSE-NEXT:    mov w9, w8
-; NOLSE-NEXT:    mvn w10, w9
-; NOLSE-NEXT:    // implicit-def: $x9
-; NOLSE-NEXT:    mov w9, w10
+; NOLSE-NEXT:    mvn w9, w9
+; NOLSE-NEXT:    // implicit-def: $x10
+; NOLSE-NEXT:    // kill: def $x9 killed $w9
 ; NOLSE-NEXT:    orr x14, x9, #0xfffffffffffffffe
 ; NOLSE-NEXT:    mov x15, #-1
 ; NOLSE-NEXT:  .LBB9_2: // %atomicrmw.start
 ; NOLSE-NEXT:    // Parent Loop BB9_1 Depth=1
 ; NOLSE-NEXT:    // => This Inner Loop Header: Depth=2
 ; NOLSE-NEXT:    ldaxp x10, x9, [x13]
 ; NOLSE-NEXT:    cmp x10, x8
 ; NOLSE-NEXT:    cset w12, ne

 ; LSE-NEXT:  .LBB9_1: // %atomicrmw.start
 ; LSE-NEXT:    // =>This Inner Loop Header: Depth=1
 ; LSE-NEXT:    ldr x10, [sp, #40] // 8-byte Folded Reload
 ; LSE-NEXT:    ldr x8, [sp, #32] // 8-byte Folded Reload
 ; LSE-NEXT:    ldr x9, [sp, #24] // 8-byte Folded Reload
 ; LSE-NEXT:    mov x0, x8
 ; LSE-NEXT:    mov x1, x10
 ; LSE-NEXT:    mov w11, w8
-; LSE-NEXT:    mvn w12, w11
-; LSE-NEXT:    // implicit-def: $x11
-; LSE-NEXT:    mov w11, w12
+; LSE-NEXT:    mvn w11, w11
+; LSE-NEXT:    // implicit-def: $x12
+; LSE-NEXT:    // kill: def $x11 killed $w11
 ; LSE-NEXT:    orr x2, x11, #0xfffffffffffffffe
 ; LSE-NEXT:    mov x11, #-1
 ; LSE-NEXT:    // kill: def $x2 killed $x2 def $x2_x3
 ; LSE-NEXT:    mov x3, x11
 ; LSE-NEXT:    caspal x0, x1, x2, x3, [x9]
 ; LSE-NEXT:    mov x9, x1
 ; LSE-NEXT:    str x9, [sp, #8] // 8-byte Folded Spill
 ; LSE-NEXT:    eor x11, x9, x10

llvm/test/CodeGen/X86/bitcast-setcc-128.ll

 ; AVX1-NEXT:    orq %rcx, %rax
 ; AVX1-NEXT:    retq
 ;
 ; AVX2-LABEL: v16i8_widened_with_ones:
 ; AVX2:       # %bb.0: # %entry
 ; AVX2-NEXT:    vpcmpeqb %xmm1, %xmm0, %xmm0
 ; AVX2-NEXT:    vpcmpeqd %xmm1, %xmm1, %xmm1
 ; AVX2-NEXT:    vinserti128 $1, %xmm1, %ymm0, %ymm0
-; AVX2-NEXT:    vpmovmskb %ymm0, %ecx
-; AVX2-NEXT:    movabsq $-4294967296, %rax # imm = 0xFFFFFFFF00000000
+; AVX2-NEXT:    vpmovmskb %ymm0, %eax
+; AVX2-NEXT:    movabsq $-4294967296, %rcx # imm = 0xFFFFFFFF00000000
 ; AVX2-NEXT:    orq %rcx, %rax
 ; AVX2-NEXT:    vzeroupper
 ; AVX2-NEXT:    retq
 ;
 ; AVX512F-LABEL: v16i8_widened_with_ones:
 ; AVX512F:       # %bb.0: # %entry
 ; AVX512F-NEXT:    vpcmpeqb %xmm1, %xmm0, %xmm0
 ; AVX512F-NEXT:    vpmovsxbd %xmm0, %zmm0