This is an archive of the discontinued LLVM Phabricator instance.

NewGVN: Relax assertion about number of times a value is seen
Needs ReviewPublic

Authored by arsenm on May 24 2021, 6:23 AM.

Details

Summary

A value can legitimately appear more than 100 times. This assert in
general seems bogus to me, but raise it to 100 times the number of
blocks in the function which is still arbitrary.

Addresses bug 50444.

Diff Detail

Unit TestsFailed

Event Timeline

arsenm created this revision.May 24 2021, 6:23 AM
arsenm requested review of this revision.May 24 2021, 6:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2021, 6:23 AM
Herald added a subscriber: wdng. · View Herald Transcript
ruiling added subscribers: asbirlea, ruiling.

Add @asbirlea since she is also working on another similar issue PR31613. sounds like the test case hit some inefficiency of the way we are processing the instructions. The %i678 = and other similar geps against %i were processed again and again.

The issue in PR31613 is a legitimate bug, since two instructions that are not congruent end up in the same congruence class based on the PredicateInfo data. Increasing the limit will not help there since the exhibited behavior in much larger apps is "out of memory", the testcase I provided is only a reduced version triggering a debuggable assertion.

I haven't looked in detail into the testcase given in this patch yet. I can believe it's possible to legitimately get to that 100 threshold and that a bigger one may be needed. I can also believe it may be dependent on the number of blocks.
However I need to more thoroughly understand the testcase and how NewGVN gets to the threshold before I can say this is the right fix, just in case the way to solve PR31613 applies here as well.
I'm also worried that if reaching any such threshold is legitimate, an assertion is not the right way to address this in the first place.

lebedev.ri resigned from this revision.Jan 12 2023, 5:21 PM

This review seems to be stuck/dead, consider abandoning if no longer relevant.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 5:21 PM

I cannot reproduce the bug. I had to decrease the counter down to 6 in order to activate the assertion. This is normal. The counter is updated after value numbering. An instruction will be processed again if one of its operands is added in a different congruence class. Do you remember which was the problem?

I cannot reproduce the bug. I had to decrease the counter down to 6 in order to activate the assertion. This is normal. The counter is updated after value numbering. An instruction will be processed again if one of its operands is added in a different congruence class. Do you remember which was the problem?

I was able to reproduce this with the testcase attached to the issue a few weeks ago, I don’t remeymore than that

We can revisit this patch if the problem shows up again.