This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Make `FoldOpIntoSelect` handle non-constants and use condition to deduce constants.
ClosedPublic

Authored by goldstein.w.n on Mar 17 2023, 7:05 PM.

Details

Summary

Make the fold use the information present in the condition for deducing constants i.e:

%c = icmp eq i8 %x, 10
%s = select i1 %c, i8 3, i8 2
%r = mul i8 %x, %s

If we fold the mul into the select, on the true side we insert 10 for %x in the mul.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Mar 17 2023, 7:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 7:05 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
goldstein.w.n requested review of this revision.Mar 17 2023, 7:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 7:05 PM
nikic requested changes to this revision.Mar 21 2023, 2:04 AM

Can you please rebase over https://github.com/llvm/llvm-project/commit/d0de2c51c9a9fc0fedb97ee98f61ce08cb34972b? I hope that will clarify what the actual change here is, because making foldOperationIntoSelectOperand() fallible doesn't make a whole lot of sense.

This revision now requires changes to proceed.Mar 21 2023, 2:04 AM

Can you please rebase over https://github.com/llvm/llvm-project/commit/d0de2c51c9a9fc0fedb97ee98f61ce08cb34972b? I hope that will clarify what the actual change here is, because making foldOperationIntoSelectOperand() fallible doesn't make a whole lot of sense.

Done and agreed. Updated version only uses the information from the condition
to improve the constant folding.

nikic edited the summary of this revision. (Show Details)Apr 10 2023, 12:58 AM

This should also relax the requirement on at least one caller so that the change becomes testable. Or maybe you can use the one in InstCombineCalls to test this, I think one of the multi-argument intrinsics should work.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1035

Why was this factored out into a separate function? Doesn't really seem to simplify things.

This should also relax the requirement on at least one caller so that the change becomes testable. Or maybe you can use the one in InstCombineCalls to test this, I think one of the multi-argument intrinsics should work.

Thats what happens in: D146350, do you want me to merge the two?

This should also relax the requirement on at least one caller so that the change becomes testable. Or maybe you can use the one in InstCombineCalls to test this, I think one of the multi-argument intrinsics should work.

Thats what happens in: D146350, do you want me to merge the two?

That patch does more than we need for this. It would be enough to relax any of the existing calls (or use the intrinsic one if it works).

Remove new helper

nikic accepted this revision.Apr 13 2023, 12:29 AM

LGTM

This revision is now accepted and ready to land.Apr 13 2023, 12:29 AM
This revision was landed with ongoing or failed builds.Apr 14 2023, 11:14 AM
This revision was automatically updated to reflect the committed changes.
nikic added a comment.May 1 2023, 12:24 AM

Sorry, I missed something in this patch as well. This transform isn't valid for undef constants: https://alive2.llvm.org/ce/z/4JjGKv We should guard it against isGuaranteedNotToBeUndefOrPoison.

goldstein.w.n added a comment.EditedMay 1 2023, 8:01 AM

Sorry, I missed something in this patch as well. This transform isn't valid for undef constants: https://alive2.llvm.org/ce/z/4JjGKv We should guard it against isGuaranteedNotToBeUndefOrPoison.

Because its constant elements can we just do C->containsUndefOrPoisonElement()?

Edit2: Edit: NVM its the condition that needs to be guarded.

Sorry, I missed something in this patch as well. This transform isn't valid for undef constants: https://alive2.llvm.org/ce/z/4JjGKv We should guard it against isGuaranteedNotToBeUndefOrPoison.

@nikic, I think D149592 should fix the issue.