This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] try to fold 'or' into 'mul' operand
ClosedPublic

Authored by spatel on Nov 29 2021, 11:24 AM.

Details

Summary

or (mul X, Y), X --> mul X, (add Y, 1) (when the multiply has no common bits with X)

We already have this fold if the pattern ends in 'add', but we can miss it if the 'add' becomes 'or' via another no-common-bits transform.

This is part of fixing:
http://llvm.org/PR49055
...but it won't make a difference on that example yet.

https://alive2.llvm.org/ce/z/Vrmoeb

Diff Detail

Event Timeline

spatel created this revision.Nov 29 2021, 11:24 AM
spatel requested review of this revision.Nov 29 2021, 11:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2021, 11:24 AM
spatel edited the summary of this revision. (Show Details)Nov 29 2021, 11:25 AM
lebedev.ri added inline comments.Nov 29 2021, 11:30 AM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2642
2644

Why APInt? This should probably be ImmConstant,
though i would think that if mul is one-use, reducing use count on X may be worthwhile still.

spatel marked 2 inline comments as done.Nov 29 2021, 12:14 PM
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2644

Yes, reducing uses of X does look better. And the 'add' version of this does that already, so it will be symmetric.

spatel updated this revision to Diff 390437.Nov 29 2021, 12:17 PM
spatel marked an inline comment as done.

Patch updated:
Removed constant restriction on the pattern - any value can be folded into an add+mul pair (and that was already shown correct in the Alive2 link).

spatel edited the summary of this revision. (Show Details)Nov 29 2021, 12:18 PM
lebedev.ri added inline comments.Nov 29 2021, 12:20 PM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2645

But if mul is not one-use yet the y is an immediate, don't we still want to fold?

spatel marked an inline comment as done.Nov 29 2021, 12:34 PM
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2645

That's not clearly better IMO. It reduces the computation chain, but we'd be replacing an 'or' with a 'mul'. That could regress codegen (especially for targets/types where the mul is not supported), and the backend can't undo that currently.

This case is shown as the 1st negative test in the patch. These are the options:

%m = mul i32 %x, 24
call void @use(i32 %m)
%r = or i32 %m, %x

vs.

%m = mul i32 %x, 24
call void @use(i32 %m)
%r = mul i32 %x, 25
lebedev.ri accepted this revision.Nov 29 2021, 12:41 PM

LG, thank you.

llvm/test/Transforms/InstCombine/or.ll
1445

Please duplicate this test with an extra use on the multiply, to complement the @mul_no_common_bits_uses.

This revision is now accepted and ready to land.Nov 29 2021, 12:41 PM
This revision was automatically updated to reflect the committed changes.
spatel marked an inline comment as done.