[PATCH] D145299: [X86] Generate better code for std::bit_ceil

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D145299#4169852 <https://reviews.llvm.org/D145299#4169852>, @spatel wrote:

> In D145299#4169799 <https://reviews.llvm.org/D145299#4169799>, @goldstein.w.n wrote:
>
>> In D145299#4169706 <https://reviews.llvm.org/D145299#4169706>, @spatel wrote:
>>
>>> In D145299#4169702 <https://reviews.llvm.org/D145299#4169702>, @RKSimon wrote:
>>>
>>>> What is preventing is from performing this in InstCombine? I don't think this pattern will emerge in SelectionDAG
>>>
>>> I haven't found a way to avoid a poison shift in IR without doing a cmp+select or umax yet. I think we're relying on the x86-specific behavior of masking the shift amount to make that part of the logic disappear in this patch.
>>
>> The IR is:
>>
>>   %2 = add i32 %0, -1
>>   %3 = tail call i32 @llvm.ctlz.i32(i32 %2, i1 false), !range !5
>>   %4 = sub nuw nsw i32 32, %3
>>   %5 = shl nuw i32 1, %4
>>   %6 = icmp ugt i32 %0, 1
>>   %7 = select i1 %6, i32 %5, i32 1
>>   ret i32 %7
>>
>> The poison shift is if `%3` is zero?
>
> Yes - if we shift by the bitwidth, that's poison in IR.
>
> IIUC in this example, we don't have to care about any input "ugt 0x8000_0000" ( https://en.cppreference.com/w/cpp/numeric/bit_ceil ), so we'd need the front-end to provide that info somehow. But 0x8000_0000 is still a valid input, so does that knowledge actually help?

yeah, the impl would either need to change `width - clz(X)` -> `-clz(X) % width` or add some assume. Guess it makes sense to keep this in X86ISel but should be sooner in the pipeline, rather than a very brittle match on `cmov`, might be able to find an easier pattern in `CombineSELECT` although have doubts about whether it would stand up in real codes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145299