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

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 ↗

(On Diff #353472)

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

612 ↗

(On Diff #353472)

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

593 ↗

(On Diff #353472)

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 ↗

(On Diff #353472)

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

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

600 ↗

(On Diff #354526)

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/DetectDeadLanes.cpp

LaneBitmask DetectDeadLanes::transferDefinedLanes(const MachineOperand &Def,
   }
   case TargetOpcode::COPY:
   case TargetOpcode::PHI:
     break;
   default:
     llvm_unreachable("function must be called with COPY-like instruction");
   }
 
-  assert(Def.getSubReg() == 0 &&
-         "Should not have subregister defs in machine SSA phase");
+  unsigned SubIdx = Def.getSubReg();
+  DefinedLanes = TRI->composeSubRegIndexLaneMask(SubIdx, DefinedLanes);
   DefinedLanes &= MRI->getMaxLaneMaskForVReg(Def.getReg());
   return DefinedLanes;
 }
 
 LaneBitmask DetectDeadLanes::determineInitialDefinedLanes(unsigned Reg) {
   // Live-In or unused registers have no definition but are considered fully
   // defined.
   if (!MRI->hasOneDef(Reg))
for (const MachineOperand &MO : DefMI.uses()) {
       unsigned OpNum = DefMI.getOperandNo(&MO);
       DefinedLanes |= transferDefinedLanes(Def, OpNum, MODefinedLanes);
     }
     return DefinedLanes;
   }
   if (DefMI.isImplicitDef() || Def.isDead())
     return LaneBitmask::getNone();
 
-  assert(Def.getSubReg() == 0 &&
-         "Should not have subregister defs in machine SSA phase");
-  return MRI->getMaxLaneMaskForVReg(Reg);
+  unsigned SubReg = Def.getSubReg();
+  return SubReg != 0 ? TRI->getSubRegIndexLaneMask(SubReg)
+                     : MRI->getMaxLaneMaskForVReg(Reg);
 }
 
 LaneBitmask DetectDeadLanes::determineInitialUsedLanes(unsigned Reg) {
   LaneBitmask UsedLanes = LaneBitmask::getNone();
   for (const MachineOperand &MO : MRI->use_nodbg_operands(Reg)) {
     if (!MO.readsReg())
       continue;
 

llvm/test/CodeGen/PowerPC/detect-dead-lanes-subreg-assertion.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-unknown \
+; RUN: -ppc-track-subreg-liveness < %s | FileCheck %s
+
+declare i64 @strlen() local_unnamed_addr
+
+define dso_local fastcc void @check_format_info_main() unnamed_addr {
+; CHECK-LABEL: check_format_info_main:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    b .LBB0_2
+; CHECK-NEXT:  # %bb.1: # %if.end40
+; CHECK-NEXT:    mflr 0
+; CHECK-NEXT:    std 0, 16(1)
+; CHECK-NEXT:    stdu 1, -32(1)
+; CHECK-NEXT:    .cfi_def_cfa_offset 32
+; CHECK-NEXT:    .cfi_offset lr, 16
+; CHECK-NEXT:    bl strlen
+; CHECK-NEXT:    nop
+; CHECK-NEXT:    extsw 3, 3
+; CHECK-NEXT:    li 4, 0
+; CHECK-NEXT:    addi 5, 3, 1
+; CHECK-NEXT:    stb 4, 0(3)
+; CHECK-NEXT:    extsw 5, 5
+; CHECK-NEXT:    stb 4, 0(5)
+; CHECK-NEXT:  .LBB0_2: # %if.then32
+entry:
+  switch i8 undef, label %if.end40 [
+    i8 0, label %if.then32
+  ]
+
+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
+}