Page MenuHomePhabricator

[GVNSink] Make GVNSink resistant against self referencing instructions (PR36954)
Needs ReviewPublic

Authored by yurai007 on Nov 15 2021, 7:15 AM.

Details

Summary

Before this change GVNSink pass suffers from stack overflow while processing self referenced instruction in unreachable basic block.
According [1] and [2] it's reasonable to make pass resistant against self referencing instructions.
To fix issue we skip sinking analysis for block in which such instruction is detected.

[1] https://groups.google.com/g/llvm-dev/c/843Tig9IzwA
[2] https://lists.llvm.org/pipermail/llvm-dev/2015-February/082629.html

Diff Detail

Event Timeline

yurai007 created this revision.Nov 15 2021, 7:15 AM
yurai007 requested review of this revision.Nov 15 2021, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2021, 7:15 AM

Skip analysis for unreachable BBs?

nikic added inline comments.Nov 16 2021, 12:27 PM
llvm/lib/Transforms/Scalar/GVNSink.cpp
367

This checks for a directly self-referential instruction, but the references may also be cyclic across multiple instructions.

As @xbolva00 mentioned, it may be simpler/cleaner to skip unreachable blocks.

llvm/test/Transforms/GVNSink/sink-common-code.ll
775

Can this test be further reduced?

yurai007 added inline comments.Nov 17 2021, 2:18 AM
llvm/lib/Transforms/Scalar/GVNSink.cpp
367

Yes, and that's what I proposed as alternative solution: https://bugs.llvm.org/show_bug.cgi?id=36954#c9 It would save some work and resolve cycles problem. The only drawback I see now is need of giving GVNSink access to DT since we need isReachableFromEntry or analogous machinery for checking reachability. I started with first, simpler approach because it was preferred by @jeroen.dobbelaere, however checking cycles with this idea during lookupOrAdd run seem to be nontrivial. So yes, I can switch to second approach and check where it leads.

yurai007 updated this revision to Diff 387894.Nov 17 2021, 3:54 AM
yurai007 edited the summary of this revision. (Show Details)
yurai007 marked an inline comment as done.