Page MenuHomePhabricator

[IfConversion] Fix diamond conversion with unanalyzable branches.
ClosedPublic

Authored by efriedma on Sep 4 2019, 5:48 PM.

Details

Summary

The code was incorrectly counting the number of identical instructions, and therefore tried to predicate an instruction which should not have been predicated. This could have various effects: a compiler crash, an assembler failure, a miscompile, or just generating an extra, unnecessary instruction.

Instead of depending on TargetInstrInfo::removeBranch, which only works on analyzable branches, just remove all branch instructions.

Fixes https://bugs.llvm.org/show_bug.cgi?id=43121 and https://bugs.llvm.org/show_bug.cgi?id=41121 .

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Sep 4 2019, 5:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2019, 5:48 PM
Herald added a subscriber: javed.absar. · View Herald Transcript
bjope added a subscriber: bjope.Sep 4 2019, 10:03 PM
dmgreen accepted this revision.Sep 5 2019, 2:49 AM

LGTM

This revision is now accepted and ready to land.Sep 5 2019, 2:49 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Sep 6 2019, 1:05 AM
hans added inline comments.
llvm/trunk/lib/CodeGen/IfConversion.cpp
1764

The previous code also decremented BBI1->NonPredSize. Do we not need to do that anymore?

(I don't know this code, just driving by.)

efriedma marked an inline comment as done.Sep 6 2019, 12:44 PM
efriedma added inline comments.
llvm/trunk/lib/CodeGen/IfConversion.cpp
1764

In theory, we probably should decrement NonPredSize, for both this loop and the following loop, to account for the instructions we're erasing.

We only use the NonPredSize as input to the if-conversion heuristics, though. It's unlikely we're going to predicate a basic block that's already predicated. So whatever we do probably has very little, if any, effect.