This is an archive of the discontinued LLVM Phabricator instance.

[ExpandPostRAPseudos] Don't add duplicate implicit operands
AcceptedPublic

Authored by foad on Apr 21 2021, 3:53 AM.

Details

Summary

When lowering COPY to a target instruction and transferring the implicit
operands, don't add any duplicate implicit operands. This affects AMDGPU
where both the original COPY and the lowered instruction like
V_MOV_B32_e32 can have an implicit use of $exec.

This is mostly just a cosmetic issue, but it could conceivably have
caused missed optimizations in
SIInstrInfo::isReallyTriviallyReMaterializable where we rely on counting
the number of implicit operands.

Diff Detail

Event Timeline

foad created this revision.Apr 21 2021, 3:53 AM
foad requested review of this revision.Apr 21 2021, 3:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2021, 3:53 AM
foad added a comment.Apr 21 2021, 3:54 AM

For AMDGPU should this be fixed earlier, by not putting the "implicit $exec" on a COPY? I don't understand whether they are required or not.

For AMDGPU should this be fixed earlier, by not putting the "implicit $exec" on a COPY? I don't understand whether they are required or not.

It was done to prevent rescheduling VGPR COPYs across EXEC modifications. Still needed I believe.

This revision is now accepted and ready to land.Apr 21 2021, 8:28 AM

For AMDGPU should this be fixed earlier, by not putting the "implicit $exec" on a COPY? I don't understand whether they are required or not.

It was done to prevent rescheduling VGPR COPYs across EXEC modifications. Still needed I believe.

I don't think the way we throw implicit exec uses on COPYs at certain points is entirely sound. It's not applied consistently (and would really need to be present on copy creation)

arsenm added inline comments.Apr 21 2021, 8:32 AM
llvm/lib/CodeGen/ExpandPostRAPseudos.cpp
72–74

This feels like a hack and we should have avoided this

I think we need to add a generic predicated copy instruction or something

foad added a comment.Apr 23 2021, 1:25 AM

For AMDGPU should this be fixed earlier, by not putting the "implicit $exec" on a COPY? I don't understand whether they are required or not.

It was done to prevent rescheduling VGPR COPYs across EXEC modifications. Still needed I believe.

I don't think the way we throw implicit exec uses on COPYs at certain points is entirely sound. It's not applied consistently (and would really need to be present on copy creation)

I guess it would be reasonable to add all the implicit exec uses at the point when control flow is lowered to exec manipulation. Any instructions created after that would need the implicit use at creation. I don't know how close we are to achieving that.

llvm/lib/CodeGen/ExpandPostRAPseudos.cpp
72–74

Not sure what you mean by "should have avoided" it. Are you saying the COPY should not have had the implicit exec use in the first place, or the MOV generated by copyPhysReg should not have had it, or TransferImplicitOperands should not exist?