Page MenuHomePhabricator

[AVR] Fix incorrect stack parameter push order
Needs ReviewPublic

Authored by Jim on Jun 9 2019, 10:31 PM.

Details

Reviewers
dylanmckay
Summary

Transform all store nodes into one single node to ensure the order of all store nodes can't be changed.
So that the push instruction sequence generated would be correct.

Diff Detail

Repository
rL LLVM

Event Timeline

Jim created this revision.Jun 9 2019, 10:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2019, 10:31 PM

Transform all store nodes into one single node to ensure the order of all store nodes can't be changed. So that the push instruction sequence generated would be correct.

The code here is probably correct - the various stores onto the stack seem to be independent - but the description here seems off. By grouping all of the stores into one TokenFactor, you're providing more ordering freedom, not less.

lib/Target/AVR/AVRISelLowering.cpp
1263

IIRC, the order of the operands of a TokenFactor don't carry any significance.

Jim added a comment.Jun 16 2019, 11:03 PM

@hfinkel
Hi, I am not really sure the means of TokenFactor. I refer the implement of other target.
From document, TokenFactor is used to represent the fact that the operand operators are independent of each other.
I am not sure why other target transform all stores into one single node by using TokenFactor.
Most of target, it uses sp+offset to store arguments into stack. So the order of store instruction doesn't matter.
In AVR, it uses push instruction to store arguments. Therefore. the order of store(push) instruction can't be changed.
Could you give me some suggestion? Thanks.

In AVR, it uses push instruction to store arguments. Therefore. the order of store(push) instruction can't be changed.

During isel, the stores are modeled using regular store instructions. And it should be possible (although maybe not efficient) to lower those stores into regular store instructions, which don't modify the stack pointer.

It looks like AVRFrameLowering::eliminateCallFramePseudoInstr has a bug: it assumes that the store instructions that are used to store arguments exist in some specific order. The target-independent backend code does not guarantee that; the stores can be rearranged during or after isel. If the stores need to be in a specific order for the store->push optimization, the AVR backend needs to rearrange them into the correct order explicitly.

x86 has a similar "push" instruction, and a specialized transform pass dedicated to optimizing store instructions to push instructions. See lib/Target/X86/X86CallFrameOptimization.cpp .