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

592

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

608

The SubIdx should not be passed through here?

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

5–30 ↗

(On Diff #353472)

IR section is not necessary

32 ↗

(On Diff #353472)

Should use a relevant name and add a descriptive comment

35–42 ↗

(On Diff #353472)

Registers section not necessary

[saghir]

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

[saghir]

llvm/lib/CodeGen/PeepholeOptimizer.cpp

592

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.

608

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 ↗

(On Diff #353472)

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

9–27

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

9–27

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

9–27

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

[arsenm]

llvm/lib/CodeGen/PeepholeOptimizer.cpp

600

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

for (unsigned i = 0, e = Uses.size(); i != e; ++i) {
         continue;
 
       // About to add uses of DstReg, clear DstReg's kill flags.
       if (!Changed) {
         MRI->clearKillFlags(DstReg);
         MRI->constrainRegClass(DstReg, DstRC);
       }
 
+      // SubReg defs are illegal in machine SSA phase,
+      // we should not generate SubReg defs.
+      //
+      // For example, for the instructions:
+      //
+      // %1:g8rc_and_g8rc_nox0 = EXTSW %0:g8rc
+      // %3:gprc_and_gprc_nor0 = COPY %0.sub_32:g8rc
+      //
+      // We should generate:
+      //
+      // %1:g8rc_and_g8rc_nox0 = EXTSW %0:g8rc
+      // %6:gprc_and_gprc_nor0 = COPY %1.sub_32:g8rc_and_g8rc_nox0
+      // %3:gprc_and_gprc_nor0 = COPY %6:gprc_and_gprc_nor0
+      //
+      if (UseSrcSubIdx)
+        RC = MRI->getRegClass(UseMI->getOperand(0).getReg());
+
       Register NewVR = MRI->createVirtualRegister(RC);
-      MachineInstr *Copy = BuildMI(*UseMBB, UseMI, UseMI->getDebugLoc(),
-                                   TII->get(TargetOpcode::COPY), NewVR)
+      BuildMI(*UseMBB, UseMI, UseMI->getDebugLoc(),
         .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();
-      }
+      if (UseSrcSubIdx)
+        UseMO->setSubReg(0);
+
       UseMO->setReg(NewVR);
       ++NumReuse;
       Changed = true;
     }
   }
 
   return Changed;
 }

llvm/test/CodeGen/PowerPC/peephole-subreg-def.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
+
+# This tests to make sure that we do not generate subreg def
+# as it is illegal to generate subreg defs in machine SSA phase.
+
+---
+name:            test_peephole_subreg_def
+alignment:       16
+tracksRegLiveness: true
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    liveins: $x3
+
+    ; CHECK-LABEL: name: test_peephole_subreg_def
+    ; CHECK: liveins: $x3
+    ; CHECK: [[COPY:%[0-9]+]]:g8rc_and_g8rc_nox0 = COPY $x3
+    ; CHECK: [[ADDI8_:%[0-9]+]]:g8rc = ADDI8 [[COPY]], 1
+    ; CHECK: [[EXTSW:%[0-9]+]]:g8rc_and_g8rc_nox0 = EXTSW [[ADDI8_]]
+    ; CHECK: [[LI8_:%[0-9]+]]:g8rc = LI8 0
+    ; CHECK: STB8 [[LI8_]], 0, [[EXTSW]]
+    ; 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_]]
+    %0:g8rc_and_g8rc_nox0 = COPY $x3
+    %1:g8rc = ADDI8 %0, 1
+    %2:g8rc_and_g8rc_nox0 = EXTSW %1
+    %3:g8rc = LI8 0
+    STB8 %3, 0, killed %2
+    %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
+
+...