This is an archive of the discontinued LLVM Phabricator instance.

PEI should be able to use backward walk in replaceFrameIndicesBackward.
ClosedPublic

Authored by alex-t on Nov 7 2022, 10:06 AM.

Details

Summary

The backward register scavenger has correct register
liveness information. PEI should leverage the backward register scavenger.

Diff Detail

Event Timeline

alex-t created this revision.Nov 7 2022, 10:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2022, 10:06 AM
alex-t requested review of this revision.Nov 7 2022, 10:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2022, 10:06 AM
arsenm added inline comments.Nov 7 2022, 10:25 AM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
1367

Can you factor out the debug instruction handling refactor in a pre-commit

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
1438

It's still dead. Even if we wanted to remove this, it doesn't belong in this patch

alex-t added inline comments.Nov 7 2022, 10:31 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
1438

It's still dead. Even if we wanted to remove this, it doesn't belong in this patch

1438

Yeah, I know. I will remove it. This is not a final change - it is just a draft.
I opened it to be able to discuss the design and solutions with all of you.

TargetRegisterInfo::eliminateFrameIndex invalidate iterator passed to it. It can remove the instruction or change the number of its operands.
I am thinking of changing its interface to make it return an iterator that points to the new/changed instruction to let caller handle this.

alex-t updated this revision to Diff 474250.Nov 9 2022, 6:14 AM

The first really working version of the frame index elimination with backward walk.

alex-t marked an inline comment as done.Nov 9 2022, 6:16 AM
foad added inline comments.Nov 9 2022, 6:20 AM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
1527

Indentation is wrong. Please run git clang-format.

1592

Please fix.

alex-t updated this revision to Diff 474331.Nov 9 2022, 12:46 PM

Formatting

alex-t marked 2 inline comments as done.Nov 9 2022, 12:47 PM
alex-t marked an inline comment as done.Nov 9 2022, 2:11 PM
alex-t added inline comments.
llvm/lib/CodeGen/PrologEpilogInserter.cpp
1367

I will change this review as soon as https://reviews.llvm.org/D137741 is submitted to the trunk.

alex-t updated this revision to Diff 474550.Nov 10 2022, 7:54 AM
alex-t marked an inline comment as done.

Common code for debug instructions frame index operand replacement is factored
out to separate function.

arsenm added inline comments.Nov 10 2022, 1:32 PM
llvm/include/llvm/CodeGen/TargetRegisterInfo.h
1031–1034

This comment isn't particularly enlightening. How about

Last time I tried this, I was trying to change all the target implementations to change the signature to make the reverse iteration less ugly. Probably shouldn't let this remain for long and just update all the targets

llvm/lib/CodeGen/PrologEpilogInserter.cpp
1455

Don't need llvm::

1458

Don't need llvm::

1505–1506

Do we have test coverage for a frame index that needs to be handled as the very first and very last instruction in the block? If not, we need them

alex-t updated this revision to Diff 475202.Nov 14 2022, 10:19 AM

Added test case for the frame index in the last instruction in the BB. "::llvm" removed.

alex-t updated this revision to Diff 475207.Nov 14 2022, 10:31 AM
alex-t marked 3 inline comments as done.

TargetRegisterInfo::supportsBackwardScavenger() description changed

alex-t marked an inline comment as done.Nov 14 2022, 10:31 AM
alex-t added inline comments.
llvm/lib/CodeGen/PrologEpilogInserter.cpp
1505–1506

We already have a case for the FI at the BB's beginning: func_add_constant_to_fi_uniform_SCC_clobber_i32 in frame-index.mir
I have added the case for the FI at the end.

arsenm accepted this revision.Nov 14 2022, 11:37 AM

LGTM. Can you create follow up patches to start migrating all the other targets?

This revision is now accepted and ready to land.Nov 14 2022, 11:37 AM

LGTM. Can you create follow up patches to start migrating all the other targets?

I am not sure if I have enough knowledge of all other architectures but I could try to work on the X86 migrating patch.

LGTM. Can you create follow up patches to start migrating all the other targets?

I am not sure if I have enough knowledge of all other architectures but I could try to work on the X86 migrating patch.

It's a mechanical change, you shouldn't need to know much beyond iterators

MaskRay added inline comments.
llvm/lib/CodeGen/PrologEpilogInserter.cpp
1455

unneeded excess blank lines

1495

LLVM style prefer --Step. Ditto elsewhere

alex-t reopened this revision.Nov 16 2022, 1:49 PM
alex-t added inline comments.
llvm/lib/CodeGen/PrologEpilogInserter.cpp
1455

I consider them to increase readability. Could you refer to some guidance with respect to which blank lines are allowed and which are not?
Or could you just mark them all with comments like this?

This revision is now accepted and ready to land.Nov 16 2022, 1:49 PM
alex-t updated this revision to Diff 475916.Nov 16 2022, 1:50 PM

TargetRegisterInfo::eliminateFrameIndex signature changed to return "true" if the MachineInstr passed in by iterator was removed

alex-t marked 2 inline comments as done.Nov 16 2022, 1:51 PM
arsenm added inline comments.Nov 17 2022, 8:34 AM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
1497

I think handling frame indexes one operand at a time is a broken API for the case where there are multiple. I think interpreting this as "removed" is too aggressive. About about returning true if the instruction was modified in place, in which case we need to resume the loop looking for additional frame indices

alex-t added inline comments.Nov 17 2022, 9:09 AM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
1497

We leave the loop only in case the instruction is removed and hence it is fully processed. We never remove instruction if it still has frame index operands to process. If the instruction is not removed (by calling eraseFromParent) we iterate through the rest of the operands looking for the FI if any.

alex-t marked an inline comment as done.Nov 17 2022, 9:39 AM
arsenm accepted this revision.Nov 17 2022, 9:46 AM
llvm/test/CodeGen/AMDGPU/spill-offset-calculation.ll