[PATCH] D111530: [TargetLowering] Optimize expanded SRL/SHL fed into SETCC ne/eq 0

Mon Nov 1 12:26:08 PDT 2021

fzhinkin added a comment.

In D111530#3083530 <https://reviews.llvm.org/D111530#3083530>, @RKSimon wrote:

> In D111530#3083495 <https://reviews.llvm.org/D111530#3083495>, @fzhinkin wrote:
>
>> Handling of trees generated during legalization of i128/i256/etc to i32 is relatively simple, but in case of i686 some of these expressions are folded into funnel shifts before `SimplifySetCC` is applied to setcc.
>> I see two options here (but I'm far from being an expert, so correct me if I'm wrong and there are simpler alternatives):
>>
>> 1. support various instructions (funnel shifts, rotations, bit/byte swaps) in `TargetLowering::optimizeSetCCOfExpandedShift` in addition to SHL/SRL;
>> 2. support only SHL/SRL in `TargetLowering::optimizeSetCCOfExpandedShift` and apply it in `DAGTypeLegalizer::IntegerExpandSetCCOperands` right after setcc's operands expansion.
>>
>> Personally I'm leaning towards second option as it should be less fragile and easier to maintain.
>
> Makes sense to try (2) first - although I expect at least partial support for (1) might end up being required - you are handling a pattern that is almost a funnel shift much of the time.

Unfortunately (2) doesn't work well, because nodes created during shift expansion may have several uses until type legalization finished, so I gave it up.

Instead I supported funnel shifts `TargetLowering::optimizeSetCCOfExpandedShift` (did not support rotations and bit/byte swaps because such nodes should not be created during expanded shift's combining).

While optimization works fine for i686 now there is an issue with AArch64: shifts expanded from types wider than `i128` won't be optimized (see `@opt_setcc_shl_ne_zero_i256`) because for AArch64 funnel shift alike patterns combined into `AArch64ISD::EXTR` instead of FSHL/FSHR. I attempted to fix it by implementing (2), but the solution was fragile and didn't work in some cases.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111530/new/

https://reviews.llvm.org/D111530