[HACK] X86: Disable isCopyInstrImpl for undef subregister defs

[arsenm]

Summary

This is a workaround for a coalescer bug where coalescing
SUBREG_TO_REG ends up losing the liveness of the high bits of the
source register. The result is an incorrect undef subregister def
instead of preserving the high values. Work around the observed
failure after the resulting mov is eliminated during allocation until
a proper fix is ready. I believe the proper fix is to make
SUBREG_TO_REG use a tied operand.

[vitalybuka]

Thanks you!

LGTM, but I am not expert on CodeGen, please look for more reviews if needed.

Would you like to add one of reproducers as a test?

• check-all is green for me
• original issue is gone
• Perf is not changed on one of our large server benchmarks

[arsenm]

I think I have a fix in the coalescer, working on refining the tests

[arsenm]

I think the real fix is to make SUBREG_TO_REG use a tied operand with a subregister but that will be a much larger patch

[arsenm]

llvm/lib/Target/X86/X86InstrInfo.cpp

3657

I should have a more general fix ready tomorrow which adds an implicit-def during coalescing. The handling here then needs to guard against implicit-defs (and report the full def super register intsead of the physical subreg undef def)

[arsenm]

Better version ends at D156346

[qcolombet]

LGTM

[RKSimon]

LGTM as a stopgap

[arsenm]

c26dfc81e254c78dc23579cf3d1336f77249e1f6

llvm/lib/Target/X86/X86InstrInfo.cpp

void X86InstrInfo::copyPhysReg(MachineBasicBlock &MBB,
 
   LLVM_DEBUG(dbgs() << "Cannot copy " << RI.getName(SrcReg) << " to "
                     << RI.getName(DestReg) << '\n');
   report_fatal_error("Cannot emit physreg copy instruction");
 }
 
 std::optional<DestSourcePair>
 X86InstrInfo::isCopyInstrImpl(const MachineInstr &MI) const {
-  if (MI.isMoveReg())
+  if (MI.isMoveReg()) {
+    // FIXME: Dirty hack for apparent invariant that doesn't hold when
+    // subreg_to_reg is coalesced with ordinary copies, such that the bits that
+    // were asserted as 0 are now undef.
+    if (MI.getOperand(0).isUndef() && MI.getOperand(0).getSubReg())
+      return std::nullopt;
+
     return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
+  }
   return std::nullopt;
 }
 
 static unsigned getLoadStoreOpcodeForFP16(bool Load, const X86Subtarget &STI) {
   if (STI.hasFP16())
     return Load ? X86::VMOVSHZrm_alt : X86::VMOVSHZmr;
   if (Load)
     return STI.hasAVX512() ? X86::VMOVSSZrm

llvm/test/CodeGen/X86/GlobalISel/add-ext.ll

 ; CHECK-NEXT:    imulq $4, %rax, %rax
 ; CHECK-NEXT:    addq %rdi, %rax
 ; CHECK-NEXT:    leal 2(%rsi), %ecx
 ; CHECK-NEXT:    movl %ecx, %ecx
 ; CHECK-NEXT:    imulq $4, %rcx, %rcx
 ; CHECK-NEXT:    addq %rdi, %rcx
 ; CHECK-NEXT:    movl (%rcx), %ecx
 ; CHECK-NEXT:    addl (%rax), %ecx
-; CHECK-NEXT:    movl %esi, %esi
-; CHECK-NEXT:    imulq $4, %rsi, %rax
+; CHECK-NEXT:    movl %esi, %eax
+; CHECK-NEXT:    imulq $4, %rax, %rax
 ; CHECK-NEXT:    addq %rdi, %rax
 ; CHECK-NEXT:    movl %ecx, (%rax)
 ; CHECK-NEXT:    retq
 
   %add1 = add nuw i32 %i, 1
   %idx1 = zext i32 %add1 to i64
   %gep1 = getelementptr i32, ptr %a, i64 %idx1
   %load1 = load i32, ptr %gep1, align 4

llvm/test/CodeGen/X86/dagcombine-cse.ll

 ; X86-NEXT:    popl %esi
 ; X86-NEXT:    popl %edi
 ; X86-NEXT:    popl %ebx
 ; X86-NEXT:    popl %ebp
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: square_high:
 ; X64:       ## %bb.0: ## %entry
-; X64-NEXT:    movl %esi, %esi
-; X64-NEXT:    movq %rsi, %rax
+; X64-NEXT:    movl %esi, %ecx
+; X64-NEXT:    movq %rcx, %rax
 ; X64-NEXT:    mulq %rdi
-; X64-NEXT:    movq %rdx, %rcx
+; X64-NEXT:    movq %rdx, %rsi
 ; X64-NEXT:    movq %rax, %r8
 ; X64-NEXT:    movq %rdi, %rax
 ; X64-NEXT:    mulq %rdi
 ; X64-NEXT:    addq %r8, %rdx
-; X64-NEXT:    movq %rcx, %rax
+; X64-NEXT:    movq %rsi, %rax
 ; X64-NEXT:    adcq $0, %rax
 ; X64-NEXT:    addq %rdx, %r8
-; X64-NEXT:    adcq %rcx, %rax
-; X64-NEXT:    imulq %rsi, %rsi
-; X64-NEXT:    addq %rax, %rsi
-; X64-NEXT:    shrdq $32, %rsi, %r8
-; X64-NEXT:    shrq $32, %rsi
+; X64-NEXT:    adcq %rsi, %rax
+; X64-NEXT:    imulq %rcx, %rcx
+; X64-NEXT:    addq %rax, %rcx
+; X64-NEXT:    shrdq $32, %rcx, %r8
+; X64-NEXT:    shrq $32, %rcx
 ; X64-NEXT:    movq %r8, %rax
-; X64-NEXT:    movq %rsi, %rdx
+; X64-NEXT:    movq %rcx, %rdx
 ; X64-NEXT:    retq
 entry:
   %conv = zext i96 %x to i192
   %mul = mul nuw i192 %conv, %conv
   %shr = lshr i192 %mul, 96
   %conv2 = trunc i192 %shr to i96
   ret i96 %conv2
 }

llvm/test/CodeGen/X86/fold-and-shift-x86_64.ll

entry:
   %tmp7 = getelementptr i8, ptr %X, i64 %tmp4
   %tmp9 = load i8, ptr %tmp7
   ret i8 %tmp9
 }
 
 define i8 @t3(ptr %X, i64 %i) {
 ; CHECK-LABEL: t3:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    movl %esi, %esi
-; CHECK-NEXT:    movzbl (%rdi,%rsi,4), %eax
+; CHECK-NEXT:    movl %esi, %eax
+; CHECK-NEXT:    movzbl (%rdi,%rax,4), %eax
 ; CHECK-NEXT:    retq
 
 entry:
   %tmp2 = shl i64 %i, 2
   %tmp4 = and i64 %tmp2, 17179869180
   %tmp7 = getelementptr i8, ptr %X, i64 %tmp4
   %tmp9 = load i8, ptr %tmp7
   ret i8 %tmp9

llvm/test/CodeGen/X86/unfold-masked-merge-scalar-constmask-lowhigh.ll

; CHECK-BMI-NEXT:    retq
   %my = and i32 %y, -65536
   %r = or i32 %mx, %my
   ret i32 %r
 }
 
 define i64 @out64_constmask(i64 %x, i64 %y) {
 ; CHECK-NOBMI-LABEL: out64_constmask:
 ; CHECK-NOBMI:       # %bb.0:
-; CHECK-NOBMI-NEXT:    movl %edi, %edi
+; CHECK-NOBMI-NEXT:    movl %edi, %ecx
 ; CHECK-NOBMI-NEXT:    movabsq $-4294967296, %rax # imm = 0xFFFFFFFF00000000
 ; CHECK-NOBMI-NEXT:    andq %rsi, %rax
-; CHECK-NOBMI-NEXT:    orq %rdi, %rax
+; CHECK-NOBMI-NEXT:    orq %rcx, %rax
 ; CHECK-NOBMI-NEXT:    retq
 ;
 ; CHECK-BMI-LABEL: out64_constmask:
 ; CHECK-BMI:       # %bb.0:
-; CHECK-BMI-NEXT:    movl %edi, %edi
+; CHECK-BMI-NEXT:    movl %edi, %ecx
 ; CHECK-BMI-NEXT:    movabsq $-4294967296, %rax # imm = 0xFFFFFFFF00000000
 ; CHECK-BMI-NEXT:    andq %rsi, %rax
-; CHECK-BMI-NEXT:    orq %rdi, %rax
+; CHECK-BMI-NEXT:    orq %rcx, %rax
 ; CHECK-BMI-NEXT:    retq
   %mx = and i64 %x, 4294967295
   %my = and i64 %y, -4294967296
   %r = or i64 %mx, %my
   ret i64 %r
 }
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;