Page MenuHomePhabricator

Fix InlineSpiller accessing not updated dominator tree base information
ClosedPublic

Authored by Ka-Ka on Dec 20 2016, 4:50 AM.

Details

Summary

The InlineSpiller was accessing the DominatorTreeBase directly through the public data member DT in the MachineDominatorTree. This is not a good idea as the "cached" information in SplitCriticalEdges is not applied before the access. The DominatorTreeBase must be accessed through the member function getBase() in MachineDominatorTree.

The fault was introduced in r266162.

I think the public data member DT in the MachineDominatorTree should have been made private in the original code (r215576) that introduced the concept of lazily updating the MachineDominatorTree information from
MachineBasicBlock::SplitCriticalEdge().

Diff Detail

Repository
rL LLVM

Event Timeline

Ka-Ka updated this revision to Diff 82091.Dec 20 2016, 4:50 AM
Ka-Ka retitled this revision from to Fix InlineSpiller accessing not updated dominator tree base information.
Ka-Ka updated this object.
Ka-Ka added reviewers: wmi, qcolombet.
Ka-Ka added subscribers: uabelho, bjope, llvm-commits.
qcolombet edited edge metadata.Dec 20 2016, 1:01 PM

Hi,

The change looks good, but do you have a test case to add with it?

Thanks,
-Quentin

Ka-Ka added a comment.Dec 20 2016, 1:35 PM

I have a reproducer for an out of target backend. I will give it a try (tomorrow) to transform it into a x86 reproducer, but I suspect that it might be hard to get it working.

Ka-Ka added a comment.Dec 29 2016, 5:12 AM

Hi,

The change looks good, but do you have a test case to add with it?

Thanks,
-Quentin

Unfortunately I'm not able to convert the reproducer we have in our out of tree backend to x86. The way split critical edges work by "caching" information in data structures (between different the passes) that is not a part of the IR make it hard to transform the reproducer to another backend as it must involve several passes. The reproducer I have in our out of tree backend work as following:

The PHIElimination pass introduce new basic blocks in PHIElimination::SplitPHIEdges(). The method MachineBasicBlock::SplitCriticalEdge() call MDT->recordSplitCriticalEdge() to update the dominator tree information, but it do not update the the dominator tree directly but instead store ("cache") the information for applying it later. Several passes execute and then when register allocator (RegAllocGreedy pass) execute InlineSpiller must access instructions in the newly introduced basic block from HoistSpillHelper::rmRedundantSpills(). The rmRedundantSpills() faulty access the dominator tree directly through the data member DT in the MachineDominatorTree (that will not apply the stored ("cached") information before the access).

To transform this reproducer to trigger the fault in another backend (x86) that have a totally different register set seems to be too tricky for me. I guess you have to either accept the changes without a testcase or leave the changes out.

Regards
/ Karl-Johan Karlsson

qcolombet accepted this revision.Jan 3 2017, 5:32 PM
qcolombet edited edge metadata.
I guess you have to either accept the changes without a testcase or leave the changes out.

That's fine, I was expecting something like this, but thanks for double checking.

LGTM.

Cheers,
-Quentin

This revision is now accepted and ready to land.Jan 3 2017, 5:32 PM
This revision was automatically updated to reflect the committed changes.