Page MenuHomePhabricator

[GlobalISel] Import patterns containing SUBREG_TO_REG
ClosedPublic

Authored by paquette on Aug 26 2019, 2:51 PM.

Details

Summary

Reuse the logic for INSERT_SUBREG to also import SUBREG_TO_REG patterns. There are some intrinsic + load/store patterns in AArch64 that use this.

  • Split inferSuperRegisterClass into two functions, one which tries to use an existing TreePatternNode (inferSuperRegisterClassForNode), and one that doesn't. SUBREG_TO_REG doesn't have a node to leverage, which is the cause for the split.
  • Rename GlobalISelEmitterInsertSubreg.td to GlobalISelEmitterSubreg.td and update it.
  • Update impacted tests in the AArch64 and X86 backends.

This is kind of a hit/miss for code size improvements/regressions. E.g. in add-ext.ll, we now get some identity copies. This isn't really anything the importer can handle, since it's caused by a later pass introducing the copy for the sake of correctness.

Diff Detail

Repository
rL LLVM

Event Timeline

paquette created this revision.Aug 26 2019, 2:51 PM
paquette updated this revision to Diff 217250.Aug 26 2019, 2:53 PM

Add missing change to a fast isel test

This is kind of a hit/miss for code size improvements/regressions. E.g. in add-ext.ll, we now get some identity copies. This isn't really anything the importer can handle, since it's caused by a later pass introducing the copy for the sake of correctness.

To clarify this for upstream after we had some private discussion: what's happening here is that the x86 GISel backend previously implemented i16->i32 anyexts with custom C++ selection as INSERT_SUBREG. With this change, we start importing the new SelectionDAG pattern which changed the anyext representation to SUBREG_TO_REG. As a result, the imul instruction which is destructive in it's first source operand, and thus relies on the TwoAddress pass to add copies, now results in an additional copy because TwoAddress commutes the source ops, causing the SUBREG_TO_REG to be the destroyed operand, and therefore we see an extra copy inserted. There isn't really anything GISel can do here it seems to avoid this. There might be the case for identity copies to be eliminated after regalloc though, it's surprising it doesn't already happen.

llvm/test/CodeGen/X86/GlobalISel/select-ext-x86-64.mir
139 ↗(On Diff #217250)

I've verified that this code is exactly what the x86 SDAG backend wants for the pattern. The original pattern seems to have been introduced by @t.p.northover in r182921. Why an anyext of GR8->i64 would want to use a mov + subreg_to_reg instead of just a subreg_to_reg I don't know.

dsanders accepted this revision.Aug 28 2019, 11:25 AM

LGTM aside from the redundant movl's. I see you've already commented on those though and given that another pass is introducing them it should probably be on that pass to avoid them if they're truly redundant

llvm/test/CodeGen/X86/GlobalISel/add-ext.ll
205 ↗(On Diff #217250)

Redundant movl instruction?

210 ↗(On Diff #217250)

Redundant movl instruction?

This revision is now accepted and ready to land.Aug 28 2019, 11:25 AM
This revision was automatically updated to reflect the committed changes.