Page MenuHomePhabricator

[MRI] isConstantPhysReg should also check if the register is clobbered by a RegMask
Needs ReviewPublic

Authored by Carrot on Aug 1 2022, 11:00 AM.

Details

Reviewers
gberry
arsenm
Summary

Current implementation of MachineRegisterInfo::isConstantPhysReg checks if the specified physical is modified in a function, but it only checks if it is used in a Def register operand, it may still be modified in a RegMask operand, like a function call. So it can give wrong result in this situation.

Other changes are used to prevent test regressions.

Diff Detail

Build Status
Buildable 188450

Event Timeline

Carrot created this revision.Aug 1 2022, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 11:00 AM
Carrot requested review of this revision.Aug 1 2022, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 11:00 AM
arsenm added inline comments.Aug 1 2022, 11:10 AM
llvm/lib/CodeGen/MachineRegisterInfo.cpp
589

This can just be a range loop. Also braces

llvm/lib/CodeGen/RegisterCoalescer.cpp
4116 ↗(On Diff #449070)

Adding a specific collection step here feels wrong. Other contexts the mask is added as soon as it's created. Not sure why we have the virtregrewriter adding these now

llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp
104 ↗(On Diff #449070)

Don't see why this would skip regmask tests

llvm/lib/Target/Mips/MipsRegisterInfo.cpp
322–323 ↗(On Diff #449070)

This should be a separate mips change

Carrot added inline comments.Aug 1 2022, 1:32 PM
llvm/lib/CodeGen/RegisterCoalescer.cpp
4116 ↗(On Diff #449070)

I agree it's ugly. Do you have any good idea where I can collect the RegMasks?

llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp
104 ↗(On Diff #449070)

To be honest, the MRI change causes two gpu tests failed, because previously there was no RegMask information collected, the checking of RegMask in isPhysRegUsed was actually nop. Now with RegMask collected, isPhysRegUsed behaves differently. I am completely ignorant with AMD GPU, so I just kept its original behavior by skipping regmask test.

Do you think it's better to enable the regmask test and send out the modified tests for review?

llvm/lib/Target/Mips/MipsRegisterInfo.cpp
322–323 ↗(On Diff #449070)

I sent out D130932 for it.

It's been a while since I looked at constant registers but I made a few changes in CHERI LLVM to avoid unnecessary copies. I just had a quick look at some of the commits and https://github.com/CTSRD-CHERI/llvm-project/commit/8588d8b81458ed6d87b674893e7752e6a6915574 looks somewhat related.

Uploaded the patch I committed to CHERI LLVM as https://reviews.llvm.org/D131958.

Can this be closed with D131958?

Can this be closed with D131958?

I think so (about to commit that revision once I check tests still pass), but it's hard to say if it covers all cases this revision does since there are no tests.

Carrot marked an inline comment as done.Tue, Sep 20, 8:01 AM

Can this be closed with D131958?

I think so (about to commit that revision once I check tests still pass), but it's hard to say if it covers all cases this revision does since there are no tests.

No, a phyiscal register is not a target constant register, but if it is not modified inside a specific function, it can still be treated as a constant register by MachineRegisterInfo::isConstantPhysReg inside the specific function. The modification should include both defined register operand and regmask.

I will update this patch. But it still depends on D132987 and D132561 to prevent regressions. These are real problems exposed by this patch.

Carrot updated this revision to Diff 462551.Fri, Sep 23, 11:04 AM

Update the patch to collect RegMask information whenever a new RegMask operand is created or its content is changed.

arsenm added inline comments.Fri, Sep 23, 1:27 PM
llvm/lib/CodeGen/RegUsageInfoPropagate.cpp
147–149

Won't this be reflected in the regmask updates (BTW I think this pass needs to be changed to directly modify the regmasks in the instruction instead of the analysis)?

Carrot added inline comments.Fri, Sep 23, 1:59 PM
llvm/lib/CodeGen/RegUsageInfoPropagate.cpp
147–149

Query register information through MRI is precise through the linked Register operands. But RegMask is different, all RegMask information is cumulated in MRI.UsedPhysRegMask. So usually we can only add new used physical registers into UsedPhysRegMask. This pass is used to reduce used physical registers, we need to reset UsedPhysRegMask and collect RegMask again.