Page MenuHomePhabricator

Allow the RegisterCoalescer to remove copies to reserved registers
ClosedPublic

Authored by hfinkel on Jan 14 2015, 2:31 PM.

Details

Summary

Hi everyone,

Please review this patch that allows the RegisterCoalescer to join "non-flipped" range pairs with a physical destination register. This allows the RegisterCoalescer to remove copies like this:

<vreg> = something (maybe a load, for example)
... (things that don't use PHYSREG)
PHYSREG = COPY <vreg>

(with all of the restrictions normally applied by the RegisterCoalescer: having compatible register classes, etc. )

Currently, RegisterCoalescer handles only the opposite case (copying *from* a physical register). I don't handle the problem fully here, but try to get the common case where there is only one use of <vreg> (the COPY).

One thing that is not clear to me: Do I need to do anything special to update the 'dead def' live range corresponding to the updated PHYSREG?

Once this functionality is committed, I'll commit a change to the PowerPC backend to make this pattern *very* common, and so it is important that the RegisterCoalescer can eliminate it when that's profitable.

Thanks again!

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 18184.Jan 14 2015, 2:31 PM
hfinkel retitled this revision from to Allow the RegisterCoalescer to remove copies to reserved registers.
hfinkel updated this object.
hfinkel edited the test plan for this revision. (Show Details)
hfinkel added reviewers: qcolombet, atrick, MatzeB.
hfinkel added a subscriber: Unknown Object (MLST).
qcolombet edited edge metadata.Jan 14 2015, 2:46 PM

Hi Hal,

I am worry about the approach here.
With this approach, we are propagating constraints on the allocation problem that we cannot recover from. We cannot split physical register live-ranges. The bottom line is it may introduce more spilling or more expensive splitting.

I would suggest instead to extend the copy propagation pass to handle such cases, since at that time the allocation has all been done. IIRC the copy propagation pass is top down, but we may be able to add a bottom up pass that would catch this.

What do you think?

Cheers,
-Quentin

Hi Hal,

I am worry about the approach here.
With this approach, we are propagating constraints on the allocation problem that we cannot recover from. We cannot split physical register live-ranges. The bottom line is it may introduce more spilling or more expensive splitting.

Good point. I was thinking about my motivating use case, where the physical register involved was reserved. In this case, removing the vreg makes the allocation problem strictly easier. Perhaps we should restrict to the case of a reserved physical register?

At least for that case, I'd like to do this before register allocation.

When I looked at our regression tests for where this changed something, it seems to pick up this case as well (on x86, rbp is reserved in that test case; on ARM, sp is reserved).

I would suggest instead to extend the copy propagation pass to handle such cases, since at that time the allocation has all been done. IIRC the copy propagation pass is top down, but we may be able to add a bottom up pass that would catch this.

I can certainly try that if you don't like my suggestion above.

Thanks again!

What do you think?

Cheers,
-Quentin

Perhaps we should restrict to the case of a reserved physical register?

Reserved registers are a different story and in that case, by all means, this is a good idea :).

I can certainly try that if you don't like my suggestion above.

I like your suggestion, the copy propagation improvement is just another angle of attack I was envisioning. It may be generally useful, but if we do not have motivating example, I would leave that for later.

Thanks,
-Quentin

Perhaps we should restrict to the case of a reserved physical register?

Reserved registers are a different story and in that case, by all means, this is a good idea :).

Good, will do :)

One more thing, I had asked:

One thing that is not clear to me: Do I need to do anything special to update the 'dead def' live range corresponding to the updated PHYSREG?

Do you know if I need to do anything specific here, or will the register replacement code that already exists take care of this in a sufficient way?

Thanks again!

I can certainly try that if you don't like my suggestion above.

I like your suggestion, the copy propagation improvement is just another angle of attack I was envisioning. It may be generally useful, but if we do not have motivating example, I would leave that for later.

Thanks,
-Quentin

Do you know if I need to do anything specific here, or will the register replacement code that already exists take care of this in a sufficient way?

Ah sorry, I forgot to reply to this as I first pushed for the copy propagation :).
The existing register replacement code should handle this case just fine. Thought that may not be well tested :P.

Thanks,
Q.

hfinkel accepted this revision.Jan 14 2015, 5:21 PM
hfinkel added a reviewer: hfinkel.

Do you know if I need to do anything specific here, or will the register replacement code that already exists take care of this in a sufficient way?

Ah sorry, I forgot to reply to this as I first pushed for the copy propagation :).
The existing register replacement code should handle this case just fine. Thought that may not be well tested :P.

Sounds good; we'll find out ;) -- Also, as it turns out, this entire thing only applies to reserved registers:

/// Return true if a copy involving a physreg should be joined.
bool RegisterCoalescer::canJoinPhys(const CoalescerPair &CP) {
  /// Always join simple intervals that are defined by a single copy from a
  /// reserved register. This doesn't increase register pressure, so it is
  /// always beneficial.
  if (!MRI->isReserved(CP.getDstReg())) {
    DEBUG(dbgs() << "\tCan only merge into reserved registers.\n");
    return false;
  }

  ...

So we're good as-is.

Thanks,
Q.

This revision is now accepted and ready to land.Jan 14 2015, 5:21 PM
hfinkel closed this revision.Jan 14 2015, 5:26 PM

r226071, thanks!