[X86FixupBWInsts] Fix miscompilation if sibling sub-register is live.

[a.elovikov]

Summary

The issues was found during D40524.

[andrew.w.kaylor]

lib/Target/X86/X86FixupBWInsts.cpp

256

Wouldn't TRI->isSuperOrSubRegisterEq() have the same effect as TRI->checkRegistersAlias()? It isn't clear to me why the new function is needed.

Also, a comment here saying more verbosely what this is meant to be looking for would be helpful. I'm thinking something like this:

// If MO is a use of any part of the destination register but is not equal to OrigDestReg
// or one of its subregisters, we cannot use SuperDestReg. For example, if OrigDestReg
// is  %al then an implicit use of %ah, %ax, %eax, or %rax will prevent us from using
// the %eax register.

[andrew.w.kaylor]

BTW, thank you very much for this patch!

[a.elovikov]

Indeed, on X86 checkRegistersAlias is just isSuperOrSubRegisterEq (although the former would be the right choice if that was not so :)).
Comment was added - thanks for the exact text!

[andrew.w.kaylor]

Looks good!

[MatzeB]

lib/Target/X86/X86FixupBWInsts.cpp

256

There is also TargetRegisterInfo::regsOverlap() which should be slightly more efficient (this is implemented in CodeGen because we have access to register units theres, which the MC layer doesn't know about)

test/CodeGen/X86/fixup-bw-inst.mir

204–246

https://llvm.org/docs/MIRLangRef.html#simplifying-mir-files

[a.elovikov]

Use regsOverlap instead of isSuperOrSubRegisterEq.

test/CodeGen/X86/fixup-bw-inst.mir

204–246

Thanks for the pointer. However, the whole file would benefit from this so I'd like to do the change in a subsequent commit. It should not require a review and can be directly committed, right?

[MatzeB]

test/CodeGen/X86/fixup-bw-inst.mir

204–246

Yes a change simplifying .mir like this is so obvious that it can be directly committed (aka post-commit review).

lib/Target/X86/X86FixupBWInsts.cpp

bool FixupBWInstPass::getSuperRegDestIfDead(MachineInstr *OrigMI,
 
   bool IsDefined = false;
   for (auto &MO: OrigMI->implicit_operands()) {
     if (!MO.isReg())
       continue;
 
     assert((MO.isDef() || MO.isUse()) && "Expected Def or Use only!");
 
-    for (MCSuperRegIterator Supers(OrigDestReg, TRI, true); Supers.isValid();
-         ++Supers) {
-      if (*Supers == MO.getReg()) {
-        if (MO.isDef())
-          IsDefined = true;
-        else
-          return false; // SuperReg Imp-used' -> live before the MI
-      }
-    }
+    if (MO.isDef() && TRI->isSuperRegisterEq(OrigDestReg, MO.getReg()))
+
+    // If MO is a use of any part of the destination register but is not equal
+    // to OrigDestReg or one of its subregisters, we cannot use SuperDestReg.
+    // For example, if OrigDestReg is %al then an implicit use of %ah, %ax,
+    // %eax, or %rax will prevent us from using the %eax register.
+    if (MO.isUse() && !TRI->isSubRegisterEq(OrigDestReg, MO.getReg()) &&
+        TRI->regsOverlap(SuperDestReg, MO.getReg()))
+      return false;
   }
   // Reg is not Imp-def'ed -> it's live both before/after the instruction.
   if (!IsDefined)
     return false;
 
   // Otherwise, the Reg is not live before the MI and the MOV can't
   // make it really live, so it's in fact dead even after the MI.
   return true;

test/CodeGen/X86/fixup-bw-inst.mir

--- |
   }
 
   define i16 @test4() {
   entry:
     %t1 = zext i1 undef to i16
     %t2 = or i16 undef, %t1
     ret i16 %t2
   }
+
+  define void @test5() {ret void}
 ...
 ---
 # CHECK-LABEL: name: test1
 name:            test1
 alignment:       4
 exposesReturnsTwice: false
 legalized:       false
 regBankSelected: false
bb.0.entry:
     successors:
     liveins: %r9d
 
     %r9b = MOV8rr undef %r10b, implicit-def %r9d, implicit killed %r9d, implicit-def %eflags
     ; CHECK: %r9b = MOV8rr undef %r10b, implicit-def %r9d, implicit killed %r9d, implicit-def %eflags
     %ax = OR16rr undef %ax, %r9w, implicit-def %eflags
     RETQ %ax
 ...
+
+---
+# CHECK-LABEL: name: test5
+name:            test5
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:
+liveins:
+  - { reg: '%ch', reg: '%bl' }
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  stackProtector:  ''
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:
+stack:
+constants:
+body:             |
+  bb.0:
+    successors:
+    liveins: %ch, %bl
+
+    %cl = MOV8rr %bl, implicit-def %cx, implicit killed %ch, implicit-def %eflags
+    ; CHECK: %cl = MOV8rr %bl, implicit-def %cx, implicit killed %ch, implicit-def %eflags
+
+    RETQ %cx
+...