Page MenuHomePhabricator

[InstCombine] treat i1 as a special type in shouldChangeType()
ClosedPublic

Authored by spatel on Jan 31 2017, 10:28 AM.

Details

Summary

This patch is based on the llvm-dev discussion here:
http://lists.llvm.org/pipermail/llvm-dev/2017-January/109631.html

Folding to i1 should always be desirable because that's better for value tracking and we have special folds for i1 types.

I checked for other users of shouldChangeType() where this might have an effect, but we already handle the i1 case differently than other types in all of those cases.

Side note: the default datalayout includes i1, so it seems we only find this gap in shouldChangeType + phi folding for the case when there is (1) an explicit datalayout without i1, (2) casting to i1 from a legal type, and (3) a phi with exactly 2 incoming casted operands (as Björn mentioned).

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Jan 31 2017, 10:28 AM

I think this makes sense; we have getBooleanContents in CodeGen, so we should generate reasonable results for i1 phi nodes even if i1 isn't considered a legal integer type.

dotdash edited edge metadata.Jan 31 2017, 1:07 PM

LGTM except for the style nitpick which is just personal preference. I don't think I'm allowed to actually accept patches though.

lib/Transforms/InstCombine/InstructionCombining.cpp
98 ↗(On Diff #86452)

I think those would be a bit more legible when written as:

bool ToLegal = ToWidth == 1 || DL.isLegalInteger(ToWidth)
spatel updated this revision to Diff 86673.Feb 1 2017, 10:59 AM
spatel marked an inline comment as done.

Patch updated:
Simplify code from cond ? true : x to cond || x.

Does one approval of the direction plus one possibly unqualified LGTM mean I can commit? :)

efriedma accepted this revision.Feb 3 2017, 12:31 PM

LGTM.

This revision is now accepted and ready to land.Feb 3 2017, 12:31 PM
This revision was automatically updated to reflect the committed changes.