This is an archive of the discontinued LLVM Phabricator instance.

[MachineScheduler] Bias physical register immediate assignments
ClosedPublic

Authored by niravd on Nov 7 2018, 11:55 AM.

Details

Summary

The machine scheduler currently biases register copies to/from
physical registers to be closer to their point of use / def to
minimize their live ranges. This change extends this to also physical
register assignments from immediate values.

This causes a reduction in reduction in overall register pressure and
minor reduction in spills and indirectly fixes an out of register assertion (PR39391).

Most test changes are from minor instruction reorderings and register
name selection changes and direct consequences of that.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Nov 7 2018, 11:55 AM

Looks reasonable to me.
Could of comments/nitpicks inlined.

llvm/lib/CodeGen/MachineScheduler.cpp
2878 ↗(On Diff #172997)

In case this instruction has more than one definition, should we check all of them?

3210 ↗(On Diff #172997)

Dead comment.

niravd updated this revision to Diff 173403.Nov 9 2018, 12:03 PM
niravd marked an inline comment as done.
niravd edited the summary of this revision. (Show Details)

Modify move immediate check to check all register definitions are physical. Also fix typo (s/isReg/getReg) that caused all register immediate assignments to be reordered.
This appears to have resolved the isntance of PR26810 in CodeGen/X86/hoist-spill.ll, but otherwise doesn't seem to have an notable effect on code generation.

srhines added a subscriber: srhines.Nov 9 2018, 5:10 PM

I see a few checks that weaken the check (e.g., -next replaced by regular checks).
Is this expected and why?

llvm/test/CodeGen/AMDGPU/insert_vector_elt.ll
20 ↗(On Diff #173403)

Given those are check-DAG, why do we have to update them?

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.class.ll
15 ↗(On Diff #173403)

Why do we need to weaken those checks?

llvm/test/CodeGen/X86/pr39391.ll
2 ↗(On Diff #173403)

Could you give a more meaningful name to the file?
Put the PR number in the comments here.

36 ↗(On Diff #173403)

Could you get rid of the implicit variables ([0-9]+)?
Use opt -instnamer for that.

niravd updated this revision to Diff 174037.Nov 14 2018, 7:17 AM
niravd marked an inline comment as done.

Resolve Quentin's comments. Revert some tests which don't need changes after previous bugfix.

niravd marked 4 inline comments as done.Nov 14 2018, 7:18 AM
niravd added inline comments.
llvm/test/CodeGen/AMDGPU/insert_vector_elt.ll
20 ↗(On Diff #173403)

It's from a limitation of FileCheck. As I understand it, FileCheck will greedily match the first of a DAG line if more than one DAG pattern will match. Putting the specialized makes sure we always associate the LOW_REG assignment with it's corresponding pattern.

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.class.ll
15 ↗(On Diff #173403)

We don't for this one; I've reverted any file that no longer needs changes.

This revision is now accepted and ready to land.Nov 14 2018, 9:14 AM
This revision was automatically updated to reflect the committed changes.
niravd marked an inline comment as done.