PeepholeOptimizer: Do not replace SubregToReg(bitcast like)

[MatzeB]

Summary

While we can usually replace bitcast like instructions
(MachineInstr::isBitcast()) with a COPY this is not legal if any of the
users is a SUBREG_TO_REG that asserts the upper bits of the result are
zero.

[qcolombet]

Hi Matthias,

That's sad that bit casts have side effects, but I agree that it seems to be the best fix. As a side question, are there anything else that check for isBitcast in other optimizations and that may have similar issues?

Anyhow, LGTM.

Cheers,
-Quentin

[MatzeB]

In D28474#640174, @qcolombet wrote:

Hi Matthias,

That's sad that bit casts have side effects, but I agree that it seems to be the best fix. As a side question, are there anything else that check for isBitcast in other optimizations and that may have similar issues?

Anyhow, LGTM.

Cheers,
-Quentin

That's the only user of MachineInstr::isBitcast(). Of course I do not know if we aren't doing replacements somewhere else that are possibly invalid because they violate the expectations of a SUBREG_TO_REG users (I would describe it as SUBREG_TO_REG breaking referential transparency that we would expect in MachineSSA...)

lib/CodeGen/PeepholeOptimizer.cpp

ValueTrackerResult ValueTracker::getNextSourceFromBitcast() {
 
   // Bail if there are effects that a plain copy will not expose.
   if (Def->hasUnmodeledSideEffects())
     return ValueTrackerResult();
 
   // Bitcasts with more than one def are not supported.
   if (Def->getDesc().getNumDefs() != 1)
     return ValueTrackerResult();
-  if (Def->getOperand(DefIdx).getSubReg() != DefSubReg)
+  const MachineOperand DefOp = Def->getOperand(DefIdx);
+  if (DefOp.getSubReg() != DefSubReg)
     // If we look for a different subreg, it means we want a subreg of the src.
     // Bails as we do not support composing subregs yet.
     return ValueTrackerResult();
 
   unsigned SrcIdx = Def->getNumOperands();
   for (unsigned OpIdx = DefIdx + 1, EndOpIdx = SrcIdx; OpIdx != EndOpIdx;
        ++OpIdx) {
     const MachineOperand &MO = Def->getOperand(OpIdx);
     if (!MO.isReg() || !MO.getReg())
       continue;
     // Ignore dead implicit defs.
     if (MO.isImplicit() && MO.isDead())
       continue;
     assert(!MO.isDef() && "We should have skipped all the definitions by now");
     if (SrcIdx != EndOpIdx)
       // Multiple sources?
       return ValueTrackerResult();
     SrcIdx = OpIdx;
   }
+
+  // Stop when any user of the bitcast is a SUBREG_TO_REG, replacing with a COPY
+  // will break the assumed guarantees for the upper bits.
+  for (const MachineInstr &UseMI : MRI.use_nodbg_instructions(DefOp.getReg())) {
+    if (UseMI.isSubregToReg())
+      return ValueTrackerResult();
+  }
+
   const MachineOperand &Src = Def->getOperand(SrcIdx);
   return ValueTrackerResult(Src.getReg(), Src.getSubReg());
 }
 
 ValueTrackerResult ValueTracker::getNextSourceFromRegSequence() {
   assert((Def->isRegSequence() || Def->isRegSequenceLike()) &&
          "Invalid definition");
 

test/CodeGen/X86/peephole.mir

+# RUN: llc -mtriple=x86_64-- -run-pass=peephole-opt %s -o - | FileCheck %s
+--- |
+  define void @func() { ret void }
+...
+---
+# Check that instructions with MI.isBitcast() are only replaced by COPY if there
+# are no SUBREG_TO_REG users.
+# CHECK-LABEL: name: func
+name: func
+registers:
+  - { id: 0, class: gr32 }
+  - { id: 1, class: fr32 }
+  - { id: 2, class: gr32 }
+
+  - { id: 3, class: gr32 }
+  - { id: 4, class: fr32 }
+  - { id: 5, class: gr32 }
+  - { id: 6, class: gr64 }
+
+body: |
+  bb.0:
+    ; CHECK: %1 = VMOVDI2SSrr %0
+    ; CHECK: %7 = COPY %0
+    ; CHECK: NOOP implicit %7
+    %0 = MOV32ri 42
+    %1 = VMOVDI2SSrr %0
+    %2 = MOVSS2DIrr %1
+    NOOP implicit %2
+
+    ; CHECK: %4 = VMOVDI2SSrr %3
+    ; CHECK-NOT: COPY
+    ; CHECK: %5 = MOVSS2DIrr %4
+    ; CHECK: %6 = SUBREG_TO_REG %5, 0
+    ; CHECK: NOOP implicit %6
+    %3 = MOV32ri 42
+    %4 = VMOVDI2SSrr %3
+    %5 = MOVSS2DIrr %4
+    %6 = SUBREG_TO_REG %5, 0, %subreg.sub_32bit
+    NOOP implicit %6
+...