This is an archive of the discontinued LLVM Phabricator instance.

[RDA] Don't visit the BB of the instruction in getUniqueReachingMIDef
ClosedPublic

Authored by samtebbs on Aug 26 2020, 3:26 AM.

Details

Summary

If the basic block of the instruction passed to getUniqueReachingMIDef
is a transitive predecessor of itself and has a definition of the
register, the function will return that definition even if it is after
the instruction given to the function. This patch stops the function
from scanning the instruction's basic block to prevent this.

Diff Detail

Event Timeline

samtebbs created this revision.Aug 26 2020, 3:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2020, 3:26 AM
samtebbs requested review of this revision.Aug 26 2020, 3:26 AM
samparker accepted this revision.Aug 26 2020, 3:30 AM

I have also stumbled across this bug while making a change so I also have this fix downstream! The existing test cases will cover this once I upload another change, so I think this is fine. Thanks!

This revision is now accepted and ready to land.Aug 26 2020, 3:30 AM

I was just writing that I understood that creating a test for this one was very difficult? I.e., creating a small test case, was that this case? Looks like Sam has one now......should it not be part of this change? But anyway, it's fine I guess.

I have also stumbled across this bug while making a change so I also have this fix downstream! The existing test cases will cover this once I upload another change, so I think this is fine. Thanks!

Thank you! I will commit after my last minute building and testing is done.

I was just writing that I understood that creating a test for this one was very difficult? I.e., creating a small test case, was that this case? Looks like Sam has one now......should it not be part of this change? But anyway, it's fine I guess.

Yeah creating a small test case for this was difficult. I believe that Sam Parker's test depends on both his changes and mine, so would be a chicken and egg problem if we were to include that into this patch.