Page MenuHomePhabricator

Fix replacedSelectWithOperand in InstCombiner to handle branch having two same successors.
Needs ReviewPublic

Authored by yinma on Oct 14 2016, 2:48 PM.

Details

Reviewers
Gerolf
apazos
Summary

The original transformation is

/ entry:
/ %4 = select i1 %3, %C* %0, %C* null
/ %5 = icmp eq %C* %4, null
/ br i1 %5, label %9, label %7
/ ...
/ ; <label>:7 ; preds = %entry
/ %8 = getelementptr inbounds %C* %4, i64 0, i32 0
/ ...
///

transformed to

/
/ %5 = icmp eq %C* %0, null
/ %6 = select i1 %3, i1 %5, i1 true
/ br i1 %6, label %9, label %7
/ ...
/ ; <label>:7 ; preds = %entry
/ %8 = getelementptr inbounds %C* %0, i64 0, i32 0 replace by %0!

However this transformation is only true when branching to two different blocks. We have to make sure this condition true in the code.

Diff Detail

Event Timeline

yinma updated this revision to Diff 74743.Oct 14 2016, 2:48 PM
yinma retitled this revision from to Fix replacedSelectWithOperand in InstCombiner to handle branch having two same successors..
yinma updated this object.
yinma updated this object.
Gerolf edited edge metadata.Oct 14 2016, 6:18 PM

This looks weird - checking for null and then ignoring it?! Could you check why this IR is generated? I suspect there is either a bug or a missing optimization. In that case I suggest getting that fixed and adding an assertion in the select optimization.

This kind of IR does actually exist. For our case, it happens after Inlining and Jump Threading, Jump Threading merges if else together and generates a branch with true/false target pointing to the same basic block. It is rare case but it happens. So we need test this situation.

apazos resigned from this revision.Feb 8 2019, 10:40 AM