Page MenuHomePhabricator

Add all constant physical registers to callee preserved masks

Authored by arichardson on Aug 16 2022, 5:18 AM.



This allows MachineCopyPropagation to eliminate copies of constant registers
such as zero registers. They were previously not being eliminated as the
check for MO.clobbersPhysReg(AvailSrc) would return true for constant
registers such as MIPS $zero.

To avoid having to manually add the zero registers to all CalleeSavedRegs
instantiations in tablegen, I instead added a new isConstant bit to the
Register and set this for MIPS, RISC-V, and AArch64 zero registers.
RegisterInfoEmitter.cpp looks at this flag and adds all constant registers
to the preserved register mask.

This may also benefit other passes but so far I have only seen differences
in MachineCopyPropagation. In the future it might make sense to generate
isConstantPhysReg() from this information.

Original source:

Diff Detail

Event Timeline

arichardson created this revision.Aug 16 2022, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2022, 5:18 AM
arichardson requested review of this revision.Aug 16 2022, 5:18 AM
luismarques added inline comments.Aug 16 2022, 5:40 AM

Redundant move

Add constant registers for the remaining architectures

arichardson added inline comments.Aug 16 2022, 5:42 AM

The previous one is in the delay-slot, so it's actually setting the argument register a0.

arichardson edited the summary of this revision. (Show Details)Aug 16 2022, 5:43 AM

Makes sense to me but somebody else should give it a more thorough review.

To avoid having to manually add the zero registers to all CalleeSavedRegs instantiations in tablegen,

It seems wise to avoid that approach, even if that also would have worked.


Oops, ignore this.

jrtc27 added inline comments.Aug 16 2022, 6:39 AM
192 ↗(On Diff #452969)

One could follow up with a way to TableGen the implementation of isConstantPhysReg

240 ↗(On Diff #452969)

let ... in would avoid the need to mess with the multiclasses but that's a matter of taste for AMDGPU maintainers

80 ↗(On Diff #452969)

I don't know about other architectures, but RISCV style would be to use let ... in (like CostPerUse below). It'd be good to make sure your change it style-consistent for each backend.

159 ↗(On Diff #452969)

I would expect Constant and Artificial to be handled the same way but I don't know why this is here...


New line before the comment like above

Add missing RISCV::VLENB

Is this actually useful in the context of CSRs?

arichardson added inline comments.Aug 16 2022, 6:40 AM
192 ↗(On Diff #452969)

Add missing RISCV::VLENB

Is this actually useful in the context of CSRs?

There are no test diffs, but it is listed by ::isConstantPhysReg, so I added it here as well.

arichardson planned changes to this revision.Aug 16 2022, 6:43 AM

Will move the tablegen changes into the commit that generates isConstantPhysReg() so that it's easier to verify that none were missed.

rebase on top of D131962

fix coding style

arichardson marked an inline comment as done.Aug 16 2022, 7:16 AM
arichardson added inline comments.

Still got a load of zero into a saved register here, not quite sure why. blt s2, s4, .LBB12_4 should be able to use the zero register.

asb added inline comments.Aug 17 2022, 5:57 AM

Yes, there's something weird going on in general with selection of the zero register. See D130809 and the bug tracking this issue. I'm not sure if it's a general problem with register coalescer or if there's some hook that's not set properly somewhere.

arsenm accepted this revision.Sep 15 2022, 3:58 PM

LGTM although this does feel a bit magical

This revision is now accepted and ready to land.Sep 15 2022, 3:58 PM
This revision was landed with ongoing or failed builds.Sep 21 2022, 5:50 AM
This revision was automatically updated to reflect the committed changes.