Page MenuHomePhabricator

Fix backtracking of instruction scheduling (pre-RA-sched)
ClosedPublic

Authored by chfast on Mar 23 2015, 10:51 AM.

Details

Reviewers
atrick
Summary

This patch fixes a bug in instruction scheduling backtracking code.
https://llvm.org/bugs/show_bug.cgi?id=22304

It can happen (by line CurSU->isPending = true; // This SU is not in AvailableQueue right now.) that a SUnit is mark as available but is not in the AvailableQueue. For SUnit being selected for scheduling both conditions must be met.

This patch mainly defensively protects from invalid removing a node from a queue. For noob like be this isAvailable flag and being in the AvailableQueue decoherence is annoying but maybe I don't have a full picture. I tried to synchronize that but it breaks the whole scheduling algorithm.

Diff Detail

Repository
rL LLVM

Event Timeline

chfast updated this revision to Diff 22486.Mar 23 2015, 10:51 AM
chfast retitled this revision from to Fix backtracking of instruction scheduling (pre-RA-sched).
chfast updated this object.
chfast edited the test plan for this revision. (Show Details)
chfast set the repository for this revision to rL LLVM.
chfast added subscribers: Unknown Object (MLST), qcolombet.
chfast updated this revision to Diff 22492.Mar 23 2015, 11:19 AM
chfast edited the test plan for this revision. (Show Details)

Tests added.

chfast added a subscriber: atrick.Mar 24 2015, 12:31 PM
atrick accepted this revision.Mar 24 2015, 4:42 PM
atrick added a reviewer: atrick.

That looks like the right fix. Please add some CHECK lines to the test to at least verify that llc is running on each function, even though you can't directly test this code path.

This revision is now accepted and ready to land.Mar 24 2015, 4:42 PM
chfast updated this revision to Diff 22630.Mar 25 2015, 3:30 AM
chfast edited edge metadata.

CHECKs added to test, they check only function names as it is generic codegen test.
I also added separated llc runs for every scheduler available. Is that ok or it will not work on some other targets?

I already accepted this. But it isn't clear to my why each CHECK-LABEL is followed by a CHECK on the same string.

chfast updated this revision to Diff 22663.Mar 25 2015, 12:27 PM

I removed unnecessary CHECKs from the test.

If you are happy with the patch, can you submit for me?

atrick closed this revision.Mar 26 2015, 8:47 PM

Author: Andrew Trick <atrick@apple.com>
Date: Thu Mar 26 20:44:13 2015

Fix a bug in SelectionDAG scheduling backtracking code: PR22304.

It can happen (by line CurSU->isPending = true; // This SU is not in
AvailableQueue right now.) that a SUnit is mark as available but is
not in the AvailableQueue. For SUnit being selected for scheduling
both conditions must be met.

This patch mainly defensively protects from invalid removing a node
from a queue. Sometimes nodes are marked isAvailable but are not in
the queue because they have been defered due to some hazard.

Patch by Pawel Bylica!

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@233351 91177308-0d34-0410-b5e6-96231b3b80d8