Page MenuHomePhabricator

Teach peephole optimizer to not emit sub-register defs
Needs ReviewPublic

Authored by saghir on Mon, May 31, 6:58 AM.

Details

Reviewers
nemanjai
arsenm
efriedma
MatzeB
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

saghir created this revision.Mon, May 31, 6:58 AM
saghir requested review of this revision.Mon, May 31, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 31, 6:58 AM

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

saghir updated this revision to Diff 348973.Tue, Jun 1, 7:33 AM

Added test case.

saghir edited the summary of this revision. (Show Details)Tue, Jun 1, 7:35 AM
saghir added reviewers: Restricted Project, 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 added inline comments.Tue, Jun 1, 2:29 PM
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 requested changes to this revision.Tue, Jun 1, 2:31 PM

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

Also could use a mir test

This revision now requires changes to proceed.Tue, Jun 1, 2:31 PM
arsenm added a comment.Tue, Jun 1, 2:33 PM

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

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 added a comment.Tue, Jun 1, 2:50 PM

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

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.

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 updated this revision to Diff 353438.Mon, Jun 21, 11:39 AM

Address comments:

  • Subreg defs are indeed illegal; taught peephole optimizer not to emit these.
  • Added mir test case.
saghir retitled this revision from DetectDeadLanes: Remove assert for subregister defs to Teach peephole optimizer to not emit sub-register defs.Mon, Jun 21, 11:41 AM
saghir edited the summary of this revision. (Show Details)
saghir updated this revision to Diff 353472.Mon, Jun 21, 1:24 PM

NFC change:
Changed the name of a variable.

arsenm added inline comments.Mon, Jun 21, 5:00 PM
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