Page MenuHomePhabricator

[InstCombine] InstCombinerImpl::visitOr - enable limited bitreverse matching
Changes PlannedPublic

Authored by RKSimon on Oct 26 2020, 10:10 AM.



Currently we only match bswap intrinsics from or(shl(),lshr()) style patterns when we could often match bitreverse intrinsics almost as cheaply.

I've currently limited this to bitreverse of i16 or less (as a similar bswap limit is i128 or less) - I'm not certain how much we should be trying to do in InstCombine vs AggressiveInstCombine (where I plan to have a similar path with no specific bitwidth limits) - should we bother with matching in InstCombine at all?

Diff Detail

Unit TestsFailed

370 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

RKSimon created this revision.Oct 26 2020, 10:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2020, 10:10 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon requested review of this revision.Oct 26 2020, 10:10 AM
nikic added a comment.Oct 31 2020, 5:06 AM

Do you have any background on where the current design comes from? If I understand right, we're currently matching bswap in InstCombine, and matching bitreverse in CGP. Why is that? Is that a compile-time consideration, or a code quality consideration (e.g., it is only profitable to materialize a bitreverse intrinsic if it is actually legal, which we do guard on in CGP)?

I think its mainly a compile-time concern - bswap is a common pattern (and we bail out in cases where we perform sub-byte operations), bitreverse is not.

nikic added a comment.Nov 1 2020, 1:40 AM

If compile-time is the only concern, then these are the numbers I get for enabling the bitreverse matching without any type size limit on CTMark: So this doesn't seem too problematic. Of course, there's the usual "death by a thousand cuts" to consider with InstCombine.

spatel added a comment.Nov 1 2020, 6:10 AM

If we want to make things better rather than just not make things worse, we should run an experiment: move the whole bswap / bitreverse matching to aggressive-instcombine without the bitwidth limit.
That may require a pass manager adjustment to run aggressive-instcombine at -O2 to avoid perf regressions, but that could pay for itself by not running these costly matchers so many times when there's really no hope of finding a match.

RKSimon planned changes to this revision.Nov 12 2020, 4:29 AM