This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Don't break the min/max idiom by mutating compares.

Authored by aemerson on May 2 2023, 11:37 PM.



This is motivated by the troubles in D143624 where a change in inlining
order resulted in instcombine seeing more code and then breaking the
smin idiom. The problem is that code like this:

%shr = ashr i32 %mul, 15
%cmp4.i = icmp sgt i32 %shr, 32767
%switch.i = icmp ult i1 %cmp4.i, true = select i1 %switch.i, i32 %shr, i32 32767

has the %cmp4.i being modified so that the idiom doesn't match. This patch
extends some of the attempts to detect a potential idiom by looking for the
"not" idiom of the %switch.i icmp in between the first cmp and the select.

This however isn't quite enough. After we prevent the first breakage, another
combine mutates the constant operand and condition of %cmp4.i, and unfortunately
that again prevents the idiom-preserving bail out code from working. To
get around this we re-use some of the canonicalization code that undoes that
transform to again prevent breakage.

I don't feel good about any of this.

Diff Detail

Event Timeline

aemerson created this revision.May 2 2023, 11:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 11:37 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
aemerson requested review of this revision.May 2 2023, 11:37 PM
nikic requested changes to this revision.May 3 2023, 8:10 AM

Yeah, no, this is too much of a hack. At that point it would be better to add a special case to the SPF matcher to handle this case.

This revision now requires changes to proceed.May 3 2023, 8:10 AM
aemerson abandoned this revision.May 4 2023, 6:02 PM

We can prevent the pattern breaking with D149918 instead of this.