Teach peephole optimizer to not emit sub-register defs

[saghir]

Summary

Peephole optimizer should not be introducing sub-reg definitions
as they are illegal in machine SSA phase. This patch modifies
the optimizer to not emit sub-register definitions.

[lebedev.ri]

Does it trigger on any of the existing tests in Codegen/PowerPC/?

[saghir]

Added test case.

[nemanjai]

I think that the original author (Matthias Braun) no longer contributes to LLVM as I haven't seen any activity from him so I'll add some reviewers that may be familiar with this code.

[nemanjai]

llvm/lib/CodeGen/DetectDeadLanes.cpp

343–344 ↗

(On Diff #348973)

Can this be conditional so we don't change the behaviour of the common case?

if (unsigned SubIdx = Def.getSubReg())
  DefinedLanes = TRI->composeSubRegIndexLaneMask(SubIdx, DefinedLanes);

[arsenm]

This assert is correct. Subregister defs in SSA are illegal, and this does run on SSA.

Also could use a mir test

[arsenm]

In D103408#2792170, @arsenm wrote:

This assert is correct. Subregister defs in SSA are illegal, and this does run on SSA.

Also could use a mir test

It's also a bug if you aren't seeing the verifier fail on this

[efriedma]

I'm not sure this is the right fix.

From my understanding, REG_SEQUENCE, INSERT_SUBREG, EXTRACT_SUBREG, and PHI should never target a subregister. For COPY, we can target a subregister, but only if that register is a physical register. So this patch only affects COPY instructions.

Given that, I think we should be bailing out sooner; trying to understand subregister defs of physical registers seems tricky, and not really related to the purpose of this pass.

[arsenm]

In D103408#2792212, @efriedma wrote:

I'm not sure this is the right fix.

From my understanding, REG_SEQUENCE, INSERT_SUBREG, EXTRACT_SUBREG, and PHI should never target a subregister. For COPY, we can target a subregister, but only if that register is a physical register.

Physical registers do not have subregister indexes. TRI::getSubReg resolves the physreg with index to a different physreg value. It's invalid to combine the two

[efriedma]

In D103408#2792223, @arsenm wrote:

In D103408#2792212, @efriedma wrote:

I'm not sure this is the right fix.

From my understanding, REG_SEQUENCE, INSERT_SUBREG, EXTRACT_SUBREG, and PHI should never target a subregister. For COPY, we can target a subregister, but only if that register is a physical register.

Physical registers do not have subregister indexes. TRI::getSubReg resolves the physreg with index to a different physreg value. It's invalid to combine the two

Oh, I see; thanks for the clarification.

[nemanjai]

It certainly does sound odd to have a subreg on a def of a virtual register in SSA. We'll see where this is coming from and why but just a quick look at the -print-after-all output reveals that it is the peephole optimizer that produces it. We'll look into whether this is a bug in the peephole optimizer or if this is some PPC callback called from the peephole optimizer that does this.

[saghir]

Address comments:

• Subreg defs are indeed illegal; taught peephole optimizer not to emit these.
• Added mir test case.

[saghir]

NFC change:
Changed the name of a variable.

[arsenm]

llvm/lib/CodeGen/PeepholeOptimizer.cpp

593

I don't see why you need to special case the use opcode

612

The SubIdx should not be passed through here?

llvm/test/CodeGen/PowerPC/peephole-subreg.mir

5–30

IR section is not necessary

32

Should use a relevant name and add a descriptive comment

35–42

Registers section not necessary

[saghir]

Simplified test case and removed the code that introduces subreg def.

[saghir]

llvm/lib/CodeGen/PeepholeOptimizer.cpp

593

Thanks, you are right, it is not really needed. I have removed the check for the opcode. I have also refined the approach to handle the case for when UseSrcSubIdx is true.

612

I think, we need to copy 32-bit subreg value from the 64-bit register. So we need to generate something like this:
%6:gprc_and_gprc_nor0 = COPY %1.sub_32:g8rc_and_g8rc_nox0

Removing SubIdx, leads to copying the complete 64-bit register and we end up generating:
%6:gprc_and_gprc_nor0 = COPY %1:g8rc_and_g8rc_nox0

which does not look right, unless I am missing something here.

llvm/test/CodeGen/PowerPC/peephole-subreg.mir

5–30

I have simplified the IR to generate the test case. However, removing the IR section, gives me error for use of undefined global value '@strlen'.

[saghir]

Added test case description.

[saghir]

llvm/test/CodeGen/PowerPC/peephole-subreg-def.mir

8–26 ↗

(On Diff #354423)

Since I renamed the file, I am writing my response to the earlier comment, of removing the IR section, here for better visibility:

"I have simplified the IR to generate the test case. However, removing the IR section, gives me error for use of undefined global value @strlen."

[saghir]

In D103408#2789491, @lebedev.ri wrote:

Does it trigger on any of the existing tests in Codegen/PowerPC/?

Sorry for the late reply, I did not see any of the existing test cases trigger on the subreg definition that peephole opt was introducing. I observed it when using -ppc-track-subreg-liveness option for certain test cases.

[arsenm]

llvm/test/CodeGen/PowerPC/peephole-subreg-def.mir

8–26 ↗

(On Diff #354423)

You can strip the strlen reference from the MIR, either by replacing with 0 or just deleting the calli. Peephole opt doesn't care about the call here

[saghir]

Removed the IR section from the test case.

[saghir]

llvm/test/CodeGen/PowerPC/peephole-subreg-def.mir

8–26 ↗

(On Diff #354423)

Thanks for your patience. I have been able to remove the IR section.

[arsenm]

llvm/lib/CodeGen/PeepholeOptimizer.cpp

621

I don't see why this second copy is necessary since %3 and %6 have the same class, but I guess that's not a big deal

llvm/lib/CodeGen/PeepholeOptimizer.cpp

if (!Uses.empty()) {
 
     // Look for PHI uses of the extended result, we don't want to extend the
     // liveness of a PHI input. It breaks all kinds of assumptions down
     // stream. A PHI use is expected to be the kill of its source values.
     for (MachineInstr &UI : MRI->use_nodbg_instructions(DstReg))
       if (UI.isPHI())
         PHIBBs.insert(UI.getParent());
 
+    bool DisableSubReg = false;
     const TargetRegisterClass *RC = MRI->getRegClass(SrcReg);
     for (unsigned i = 0, e = Uses.size(); i != e; ++i) {
       MachineOperand *UseMO = Uses[i];
       MachineInstr *UseMI = UseMO->getParent();
+
+      // Sub-reg defs are illegal in machine SSA phase,
+      // so we should generate defs without subreg.
+      //
+      // For example, for the instructions:
+      //
+      // %2:g8rc_and_g8rc_nox0 = EXTSW %1:g8rc
+      // %4:gprc_and_gprc_nor0 = COPY %1.sub_32:g8rc
+      //
+      // We should generate:
+      //
+      // %7:gprc_and_gprc_nor0 = COPY %2.sub_32:g8rc_and_g8rc_nox0
+      // %4:gprc_and_gprc_nor0 = COPY %7:gprc_and_gprc_nor0
+      //
+      if (UseSrcSubIdx && (UseMI->getOpcode() == TargetOpcode::COPY)) {
+        UseSrcSubIdx = false;
+        DisableSubReg = true;
+        RC = MRI->getRegClass(UseMI->getOperand(0).getReg());
+      }
+
       MachineBasicBlock *UseMBB = UseMI->getParent();
       if (PHIBBs.count(UseMBB))
         continue;
 
       // About to add uses of DstReg, clear DstReg's kill flags.
       if (!Changed) {
         MRI->clearKillFlags(DstReg);
         MRI->constrainRegClass(DstReg, DstRC);
       }
 
       Register NewVR = MRI->createVirtualRegister(RC);
       MachineInstr *Copy = BuildMI(*UseMBB, UseMI, UseMI->getDebugLoc(),
                                    TII->get(TargetOpcode::COPY), NewVR)
         .addReg(DstReg, 0, SubIdx);
       // SubIdx applies to both SrcReg and DstReg when UseSrcSubIdx is set.
       if (UseSrcSubIdx) {
         Copy->getOperand(0).setSubReg(SubIdx);
         Copy->getOperand(0).setIsUndef();
+      } else if (DisableSubReg) {
+        UseMO->setSubReg(0);
       }
       UseMO->setReg(NewVR);
       ++NumReuse;
       Changed = true;
     }
   }
 
   return Changed;
 }
 
 /// If the instruction is a compare and the previous instruction it's comparing

llvm/test/CodeGen/PowerPC/peephole-subreg.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=powerpc64le -simplify-mir -verify-machineinstrs \
+# RUN:   -run-pass=peephole-opt %s -o - | FileCheck %s
+
+--- |
+  declare i64 @strlen() local_unnamed_addr
+
+  define dso_local fastcc void @check_format_info_main() unnamed_addr {
+  entry:
+    %0 = zext i8 undef to i32
+    %cond = icmp eq i32 %0, 0
+    br i1 %cond, label %if.then32, label %if.end40
+
+  if.then32:                                        ; preds = %entry
+    unreachable
+
+  if.end40:                                         ; preds = %entry
+    %call413 = call i64 @strlen()
+    %sext1679 = shl i64 %call413, 32
+    %idxprom418 = ashr exact i64 %sext1679, 32
+    %arrayidx419 = getelementptr inbounds [256 x i8], [256 x i8]* null, i64 0, i64 %idxprom418
+    store i8 0, i8* %arrayidx419, align 1
+    %sext1680 = add i64 %sext1679, 4294967296
+    %idxprom420 = ashr exact i64 %sext1680, 32
+    %arrayidx421 = getelementptr inbounds [256 x i8], [256 x i8]* null, i64 0, i64 %idxprom420
+    store i8 0, i8* %arrayidx421, align 1
+    unreachable
+  }
+
+...
+---
+name:            check_format_info_main
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: crbitrc }
+  - { id: 1, class: g8rc }
+  - { id: 2, class: g8rc_and_g8rc_nox0 }
+  - { id: 3, class: g8rc }
+  - { id: 4, class: gprc_and_gprc_nor0 }
+  - { id: 5, class: gprc }
+  - { id: 6, class: g8rc_and_g8rc_nox0 }
+frameInfo:
+  maxAlignment:    1
+  hasCalls:        true
+machineFunctionInfo: {}
+body:             |
+  ; CHECK-LABEL: name: check_format_info_main
+  ; CHECK: bb.0.entry:
+  ; CHECK:   successors: %bb.1(0x30000000), %bb.2(0x50000000)
+  ; CHECK:   [[CRUNSET:%[0-9]+]]:crbitrc = CRUNSET
+  ; CHECK:   BC killed [[CRUNSET]], %bb.2
+  ; CHECK:   B %bb.1
+  ; CHECK: bb.1.if.then32:
+  ; CHECK:   successors:
+  ; CHECK: bb.2.if.end40:
+  ; CHECK:   liveins: $x2
+  ; CHECK:   ADJCALLSTACKDOWN 32, 0, implicit-def dead $r1, implicit $r1
+  ; CHECK:   BL8_NOP @strlen, csr_ppc64_altivec, implicit-def dead $lr8, implicit $rm, implicit $x2, implicit-def $r1, implicit-def $x3
+  ; CHECK:   ADJCALLSTACKUP 32, 0, implicit-def dead $r1, implicit $r1
+  ; CHECK:   [[COPY:%[0-9]+]]:g8rc = COPY $x3
+  ; CHECK:   [[EXTSW:%[0-9]+]]:g8rc_and_g8rc_nox0 = EXTSW [[COPY]]
+  ; CHECK:   [[LI8_:%[0-9]+]]:g8rc = LI8 0
+  ; CHECK:   STB8 [[LI8_]], 0, [[EXTSW]] :: (store 1 into %ir.arrayidx419)
+  ; CHECK:   [[COPY1:%[0-9]+]]:gprc_and_gprc_nor0 = COPY [[EXTSW]].sub_32
+  ; CHECK:   [[COPY2:%[0-9]+]]:gprc_and_gprc_nor0 = COPY [[COPY1]]
+  ; CHECK:   [[ADDI:%[0-9]+]]:gprc = ADDI killed [[COPY2]], 1
+  ; CHECK:   [[EXTSW_32_64_:%[0-9]+]]:g8rc_and_g8rc_nox0 = EXTSW_32_64 killed [[ADDI]]
+  ; CHECK:   STB8 [[LI8_]], 0, killed [[EXTSW_32_64_]] :: (store 1 into %ir.arrayidx421)
+  bb.0.entry:
+    successors: %bb.1(0x30000000), %bb.2(0x50000000)
+
+    %0:crbitrc = CRUNSET
+    BC killed %0, %bb.2
+    B %bb.1
+
+  bb.1.if.then32:
+    successors:
+
+
+  bb.2.if.end40:
+    liveins: $x2
+    ADJCALLSTACKDOWN 32, 0, implicit-def dead $r1, implicit $r1
+    BL8_NOP @strlen, csr_ppc64_altivec, implicit-def dead $lr8, implicit $rm, implicit $x2, implicit-def $r1, implicit-def $x3
+    ADJCALLSTACKUP 32, 0, implicit-def dead $r1, implicit $r1
+    %1:g8rc = COPY $x3
+    %2:g8rc_and_g8rc_nox0 = EXTSW %1
+    %3:g8rc = LI8 0
+    STB8 %3, 0, killed %2 :: (store 1 into %ir.arrayidx419)
+    %4:gprc_and_gprc_nor0 = COPY %1.sub_32
+    %5:gprc = ADDI killed %4, 1
+    %6:g8rc_and_g8rc_nox0 = EXTSW_32_64 killed %5
+    STB8 %3, 0, killed %6 :: (store 1 into %ir.arrayidx421)
+
+...