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–345

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))
     return LaneBitmask::getAll();
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;