This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Reintroduce ARGUMENT moving logic
ClosedPublic

Authored by sunfish on Dec 9 2015, 6:08 AM.

Details

Summary

Reinteroduce the code for moving ARGUMENTS back to the top of the basic block. While the ARGUMENTS physical register prevents sinking and scheduling from moving them, it does not appear to be sufficient to prevent SelectionDAG from moving them down in the initial schedule. This patch introduces a patch that moves them back to the top immediately after SelectionDAG runs.

This is still hopefully a temporary solution. http://reviews.llvm.org/D14750 is one alternative, though the review has not been favorable, and proposed alternatives are longer-term and have other downsides.

This fixes the main outstanding -verify-machineinstrs failures, so it adds -verify-machineinstrs to several tests.

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish updated this revision to Diff 42290.Dec 9 2015, 6:08 AM
sunfish retitled this revision from to [WebAssembly] Reintroduce ARGUMENT moving logic.
sunfish updated this object.
sunfish set the repository for this revision to rL LLVM.
sunfish added a subscriber: llvm-commits.
jfb added a comment.Dec 9 2015, 6:41 AM

The order of the ARGUMENTs isn't changed by SelectionDAG?

This seems to fix the issue, but I'm not familiar enough with SelectionDAG to have an opinion about what it should be doing.

lib/Target/WebAssembly/WebAssemblyArgumentMove.cpp
57

Dead.

82

Factor out isArgument()?

sunfish updated this revision to Diff 42299.Dec 9 2015, 7:11 AM

Address review comments.

sunfish marked 2 inline comments as done.Dec 9 2015, 7:13 AM

SelectionDAG's last task is to convert from its DAG representation to linear MachineInstrs.

Changing the way it handles physical register liveness would also be a longer-term project.

jfb accepted this revision.Dec 9 2015, 7:16 AM
jfb added a reviewer: jfb.

lgtm, with the caveat that I don't fully understand the tradeoff in going one way or the other (besides timing).

This revision is now accepted and ready to land.Dec 9 2015, 7:16 AM
This revision was automatically updated to reflect the committed changes.