This is an archive of the discontinued LLVM Phabricator instance.

[JumpThreading] [PR40992] Fix miscompile when folding a successor of an indirectbr
ClosedPublic

Authored by brzycki on Apr 4 2019, 1:51 PM.

Details

Summary

The continue statement in JumpThreadingPass::ProcessThreadableEdges() correctly identifies the case when a predecessor is an indirectbr or callbr and must not be threaded. Unfortunately the bookkeeping for the remaining edges was incorrect and there are cases where a conditional branch in BB may be incorrectly folded based on the flawed analysis above. This patch correctly prevents folding in these cases while not fully pessimizing other threading opportunities to the same BB.

Thanks to Matthias Liedtke for creating a simplified test-case and concrete failure example.

See PR40992 for debug analysis.

Diff Detail

Repository
rL LLVM

Event Timeline

brzycki created this revision.Apr 4 2019, 1:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 1:51 PM
Herald added subscribers: jfb, hiraditya. · View Herald Transcript

Passes make check and test-suite runs.

craig.topper added inline comments.Apr 4 2019, 2:01 PM
llvm/lib/Transforms/Scalar/JumpThreading.cpp
1666 ↗(On Diff #193773)

Can we just use PredToDestList.size() here? And switch to BB->hasNPredecessors(PredToDestList.size()). pred_size is linear in the number of predecessors, but hasNPredecessors will only count to N and return false if that's not the end without continuing to the end.

brzycki marked an inline comment as done.Apr 4 2019, 2:07 PM
brzycki added inline comments.
llvm/lib/Transforms/Scalar/JumpThreading.cpp
1666 ↗(On Diff #193773)

That's fine by me. I only wrote it this way to make minimal changes. I'll try your suggested change and test for correctness.

brzycki updated this revision to Diff 193781.Apr 4 2019, 2:19 PM

Address review comments.

brzycki updated this revision to Diff 193785.Apr 4 2019, 2:26 PM

Added missing check to testcase ensuring %bb1 threads to %bb7 when %c is false.

E5ten added a subscriber: E5ten.Apr 4 2019, 4:23 PM
sebpop accepted this revision.Apr 5 2019, 9:14 AM

Looks good to me.

This revision is now accepted and ready to land.Apr 5 2019, 9:14 AM
This revision was automatically updated to reflect the committed changes.