The backward register scavenger has correct register
liveness information. PEI should leverage the backward register scavenger.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
llvm/lib/CodeGen/PrologEpilogInserter.cpp | ||
---|---|---|
1360 | I will change this review as soon as https://reviews.llvm.org/D137741 is submitted to the trunk. |
Common code for debug instructions frame index operand replacement is factored
out to separate function.
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 | ||
1369 | Don't need llvm:: | |
1416–1417 | 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 | |
1448 | Don't need llvm:: |
Added test case for the frame index in the last instruction in the BB. "::llvm" removed.
llvm/lib/CodeGen/PrologEpilogInserter.cpp | ||
---|---|---|
1416–1417 | 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 am not sure if I have enough knowledge of all other architectures but I could try to work on the X86 migrating patch.
llvm/lib/CodeGen/PrologEpilogInserter.cpp | ||
---|---|---|
1366 | I consider them to increase readability. Could you refer to some guidance with respect to which blank lines are allowed and which are not? |
TargetRegisterInfo::eliminateFrameIndex signature changed to return "true" if the MachineInstr passed in by iterator was removed
llvm/lib/CodeGen/PrologEpilogInserter.cpp | ||
---|---|---|
1408 | 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 |
llvm/lib/CodeGen/PrologEpilogInserter.cpp | ||
---|---|---|
1408 | 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. |
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