Page MenuHomePhabricator

[PredicateInfo] Do not process unreachable operands.
ClosedPublic

Authored by twoh on Apr 25 2019, 2:31 PM.

Details

Summary

We should excluded unreachable operands from processing as their DFS visitation order is undefined. When renameUses function sorts OpsToRename (https://fburl.com/d2wubn60), the comparator assumes that the parent block of the operand has a corresponding dominator tree node. This is not the case for unreachable operands and crashes the compiler.

Diff Detail

Repository
rL LLVM

Event Timeline

twoh created this revision.Apr 25 2019, 2:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2019, 2:31 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
twoh added a comment.May 3 2019, 9:42 AM

Friendly ping. Thanks!

twoh added a comment.May 14 2019, 11:48 AM

Friendly ping again. This is a fix for a compiler crash.

Testcase?

For any non-PHI instruction that is reachable, all of its operands must also be reachable. We can use this to reduce the number of places we need to check for reachability. I'm pretty sure the new checks in processSwitch and processBranch are useless: iterating over the domtree will not visit unreachable blocks. And we should probably add a check to PredicateInfo::buildPredicateInfo to avoid processing assumes in unreachable code, rather than checking the reachability of the operands of an assume.

twoh updated this revision to Diff 199648.May 15 2019, 11:19 AM

Thank you for your comments @efriedma! I addressed your comments and added a test case. The test segfaults clang without proposed change.

efriedma accepted this revision.May 15 2019, 11:59 AM

LGTM with one minor comment.

llvm/lib/Transforms/Utils/PredicateInfo.cpp
476 ↗(On Diff #199648)

It would be more clear to use isReachableFromEntry (which does the same thing, but it has a better name).

This revision is now accepted and ready to land.May 15 2019, 11:59 AM
twoh updated this revision to Diff 199657.May 15 2019, 12:22 PM

Use isReachableFromEntry. Thank you for the suggestion!

This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.May 15 2019, 1:52 PM