This is an archive of the discontinued LLVM Phabricator instance.

[mips] Reordering callseq* nodes to be linear
ClosedPublic

Authored by abeserminji on Aug 31 2017, 3:22 AM.

Details

Summary

When calling a function and passing large argument by value, and when argument size is above certain threshold, memcpy is used to copy argument on stack instead of sequence of loads and stores. In that case callseq* nodes for memcpy are nested inside callseq* nodes for the called function. This patch corrects this behavior by moving callseq_start of the called function after arguments calculation to temporary registers, so that callseq* nodes in resulting DAG are linear.

Diff Detail

Repository
rL LLVM

Event Timeline

abeserminji created this revision.Aug 31 2017, 3:22 AM
sdardis requested changes to this revision.Sep 5 2017, 4:41 AM

+llvm-commits

That list should have been added when the patch was uploaded.

Can you re-upload the patch with full context: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Can you also add -verify-machineinstrs to test/CodeGen/Mips/largeimmprinting.ll ? This is the existing test case that breaks the machine verifier.

test/CodeGen/Mips/callseq_order.ll
19–53

Rather than matching SelectionDAG's output, can you instead match the end output of -debug-only=isel and ensure that the output is a sequence of ADJCALLSTACKDOWN, ADJCALLSTACKUP which are not nested and match the memcpy calls where they occur.

This revision now requires changes to proceed.Sep 5 2017, 4:41 AM
abeserminji edited edge metadata.
abeserminji marked an inline comment as done.

Comments resolved.

Forgot to upload patch with full context.

sdardis added inline comments.Sep 20 2017, 6:22 AM
test/CodeGen/Mips/callseq_order.ll
2–9

Add -verify-machineinstrs to these llc commands.

2–9

These can have -stop-before=expand-isel-pseudos to terminate llc early.

16

This and the one below can be removed.

17

The #0 and the #1s below can be removed.

abeserminji marked 4 inline comments as done.

Comments resolved.

sdardis accepted this revision.Sep 26 2017, 3:10 AM

LGTM with nits addressed.

test/CodeGen/Mips/callseq_order.ll
1

You can drop the CPU specification portion of the llc invocations here, the defaults of 32r2 and 64r2 should sufficient to test the logic that has been changed.

test/CodeGen/Mips/llvm-ir/mul.ll
271

You don't need to bind the register number to a FileCheck variable in this case, as it's unused afterwards. Either match it with {{[0-9a-z]+}} or drop the register portion as we're interested in matching the:

lw $25, %call16(__multi3)

part of the instruction. This applies to the following test changes as well.

This revision is now accepted and ready to land.Sep 26 2017, 3:10 AM
abeserminji marked 2 inline comments as done.

Comments resolved.

This revision was automatically updated to reflect the committed changes.