Page MenuHomePhabricator

[InstCombine] Fold freeze into phi if one operand is not undef
ClosedPublic

Authored by aqjune on Sun, Jul 26, 10:30 AM.

Details

Summary

This patch adds folding freeze into phi if it has only one operand to target.

Diff Detail

Event Timeline

aqjune created this revision.Sun, Jul 26, 10:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptSun, Jul 26, 10:30 AM
nikic added inline comments.Sun, Jul 26, 2:37 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1053

Won't this let through something like a constant vector with one undef element? Maybe just use the usual guaranteed-not-undef query here?

llvm/test/Transforms/InstCombine/freeze-phi.ll
216

Can you also add a test with an all-constant phi, in which case the freeze will get dropped completely? I believe the code also handles that case.

aqjune updated this revision to Diff 280765.Sun, Jul 26, 5:47 PM

Use isGuaranteedNotToBeUndefOrPoison to correctly deal with constant vectors

aqjune marked 3 inline comments as done.Sun, Jul 26, 5:49 PM
aqjune added inline comments.
llvm/test/Transforms/InstCombine/freeze-phi.ll
216

Seems like InstSimplify already deals with the case: function const in this file has a phi with constants.

aqjune retitled this revision from [InstCombine] Fold freeze into phi if beneficial to [InstCombine] Fold freeze into phi if one operand is not undef.Sun, Jul 26, 7:23 PM
nikic accepted this revision.Mon, Jul 27, 12:35 AM

LGTM

As a possible followup: This code could be generalized a bit by using "constant && !constexpr" for non-freeze and "guaranteed-not-undef" (without any constant checks) for freeze, with some appropriate renames. This would basically change the transform from splitting into "constant" and "non-constant" inputs, into "foldable" and "non-foldable" inputs (where "foldable = constant" for most cases).

llvm/test/Transforms/InstCombine/freeze-phi.ll
216

Oh right, that makes sense. In that case the whole phi is "guaranteed not undef", so we don't ever get here.

This revision is now accepted and ready to land.Mon, Jul 27, 12:35 AM
This revision was automatically updated to reflect the committed changes.
aqjune marked an inline comment as done.

It makes sense; freeze is simply no-op for a non-undef/poison constexpr, so it should work.