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 ↗(On Diff #218968)

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 ↗(On Diff #218968)

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.