Page MenuHomePhabricator

[MachineCopyPropagation] Incorrect COPY removal in AArch64 backend
ClosedPublic

Authored by HaoLiu on Mar 10 2015, 8:01 PM.

Details

Reviewers
jmolloy
Jiangning
Summary

This bug is happened in AArch64 backend when there are machine instruction sequences as follows:

(a)   %Q5_Q6<def> = COPY %Q2_Q3
(b)   %D5<def> =
(c)   %D3<def> =
(d)   %D3<def> = COPY %D6              // Incorrectly removed in MachineCopyPropagation
(e)   use of %D3

The MachineCopyPropagation uses a map called AvailCopyMap to store the available COPY instructions of specific registers. The situations for executing the above sequences are as follows:
(1) After visiting instruction (a), all sub registers of Q5_Q6 are mapped to (a) in AvailCopyMap:

Q5_Q6  - (a)
Q5         - (a)
D5         - (a)
Q6         - (a)
D6         - (a)

(2) After visiting instruction (b), as register D5 is redefined, all registers aliasing to D5 will be cleared in the AvailCopyMap. Then the Map is like:

D6         - (a)
Q6         - (a)

(3) When visiting instruction (c), D3 is redefined. This pass tries to remove all COPYs related to instruction (a). But such removal logic is as follows:

if (AvailCopyMap.erase(MappedDef)) {
  for (MCSubRegIterator SR(MappedDef, TRI); SR.isValid(); ++SR)
    AvailCopyMap.erase(*SR);
}

The MappedDef is register Q5_Q6, as it has already been removed in step (2), the for loop will never be executed. As a result, the AvailCopyMap is kept the same. We still have

D6         - (a)
Q6         - (a)

(4) When visiting instruction (d), it find an available COPY in the Map and remove (d).
(5) As D3 is not as expected, the test will have an incorrect result.

This fix is to make sure we remove all related COPYs in the Map in step (3). So we just need to remove the if statement.
The test case is a bit complex, as we need the specific instruction sequences to reproduce this bug.

Ask for code review.

Thanks,
-Hao

Diff Detail

Event Timeline

HaoLiu updated this revision to Diff 21672.Mar 10 2015, 8:01 PM
HaoLiu retitled this revision from to [MachineCopyPropagation] Incorrect COPY removal in AArch64 backend.
HaoLiu updated this object.
HaoLiu edited the test plan for this revision. (Show Details)
HaoLiu added reviewers: jmolloy, Jiangning.
HaoLiu added a subscriber: Unknown Object (MLST).
jmolloy accepted this revision.Mar 12 2015, 7:15 AM
jmolloy edited edge metadata.

Hi Hao,

This looks correct to me. The original logic was certainly faulty.

Cheers,

James

This revision is now accepted and ready to land.Mar 12 2015, 7:15 AM
HaoLiu closed this revision.Mar 15 2015, 7:18 PM