This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] add recursion depth param to matchSelectPattern
ClosedPublic

Authored by spatel on Jan 23 2018, 2:52 PM.

Details

Summary

We're getting bug reports:
https://bugs.llvm.org/show_bug.cgi?id=35807
https://bugs.llvm.org/show_bug.cgi?id=35840
https://bugs.llvm.org/show_bug.cgi?id=36045
...where we blow up the stack in value tracking because other passes are sending in selects that have an operand that is itself the select.

We don't currently have a reliable way to avoid analyzing dead code that may take non-standard forms, so bail out when things go too far.

Diff Detail

Event Timeline

spatel created this revision.Jan 23 2018, 2:52 PM

As I understand it, we want to uncover these bugs in passes that use value tracking because it signals a potential efficiency improvement for them when dealing with dead code or other corner cases.

We don't actually have a solution to reliably avoid analyzing dead code... see the discussion on https://reviews.llvm.org/D34135.

As I understand it, we want to uncover these bugs in passes that use value tracking because it signals a potential efficiency improvement for them when dealing with dead code or other corner cases.

We don't actually have a solution to reliably avoid analyzing dead code... see the discussion on https://reviews.llvm.org/D34135.

Ah...so should we just bail out here in value tracking then?

Well, checking explicitly for a select which directly refers to itself doesn't really solve anything; you can have exactly the same issue with indirect references.

Let's just add a recursion limit for now, I think.

spatel updated this revision to Diff 131167.Jan 23 2018, 4:13 PM
spatel retitled this revision from [ValueTracking] add an assert to prevent infinite recursion to [ValueTracking] add recursion depth param to matchSelectPattern .
spatel edited the summary of this revision. (Show Details)

I think this looks okay, but let's wait to see if @davide has any input.

This revision is now accepted and ready to land.Jan 23 2018, 5:15 PM
This revision was automatically updated to reflect the committed changes.