This is an archive of the discontinued LLVM Phabricator instance.

[AggressiveInstCombine] Add `udiv` and `urem` instrs to TruncInstCombine DAG
ClosedPublic

Authored by anton-afanasyev on Sep 9 2021, 8:49 AM.

Details

Summary

Add udiv and urem instructions to the DAG post-dominated by trunc,
allowing TruncInstCombine to reduce bitwidth of expressions containing these
instructions. It is sufficient to require that all truncated bits of both
operands are zeros: https://alive2.llvm.org/ce/z/yiithn
(urem case is identical).

Diff Detail

Event Timeline

anton-afanasyev created this revision.Sep 9 2021, 8:49 AM
anton-afanasyev requested review of this revision.Sep 9 2021, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 8:49 AM

Seems reasonable.

llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
152
315–324
anton-afanasyev marked 2 inline comments as done.

Address comments

lebedev.ri accepted this revision.Sep 10 2021, 8:11 AM

LG, thanks.

llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
318–319

Hm, if you swap arguments around, does it lead to better clang-format?

This revision is now accepted and ready to land.Sep 10 2021, 8:11 AM
llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
152

Ok, added to todo.

315–324

Ok, done.

318–319

No(
It fixes even my "legal" trials like this one:

MinBitWidth = std::max(Known.getMaxValue().getActiveBits(),
                       MinBitWidth);
spatel added inline comments.Sep 10 2021, 8:40 AM
llvm/test/Transforms/AggressiveInstCombine/trunc_udivrem.ll
22

Typo: 'zext i16 %y to i32'

113

Typo: 'zext i16 %y to i32'

anton-afanasyev marked 3 inline comments as done.Sep 10 2021, 8:48 AM
anton-afanasyev added inline comments.
llvm/test/Transforms/AggressiveInstCombine/trunc_udivrem.ll
22

Oops, thanks, fixed!

113

Oops, thanks, fixed!

anton-afanasyev marked 2 inline comments as done.

Fix test

spatel accepted this revision.Sep 10 2021, 9:02 AM

LGTM

llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
152

Hi @lebedev.ri, @spatel , according to this patch rGb289dc530632, sdiv/srem (as well as udiv/urem) instrs are guarded with trunc/ext wherever it's possible. So it looks like support them here in AIC is redundant as we get always MinBitWidth == OrigBitWidth after correlated value propagation.
So I'm leaning towards reverting this patch. What do you think?