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

[RKSimon]

Summary

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?

[nikic]

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)?

[RKSimon]

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]

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: http://llvm-compile-time-tracker.com/compare.php?from=27f647d117087ca11959e232e6443f4aee31e966&to=b9042c581410835940a253af86646edeabfc858e&stat=instructions So this doesn't seem too problematic. Of course, there's the usual "death by a thousand cuts" to consider with InstCombine.

[spatel]

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.