[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

Thu Dec 10 23:42:22 PST 2020

lebedev.ri added a subscriber: t.p.northover.
lebedev.ri added a comment.

In D87188#2447025 <https://reviews.llvm.org/D87188#2447025>, @craig.topper wrote:

> In D87188#2446096 <https://reviews.llvm.org/D87188#2446096>, @spatel wrote:
>
>> In D87188#2445506 <https://reviews.llvm.org/D87188#2445506>, @lebedev.ri wrote:
>>
>>> Partial rebase (without updating test coverage)
>>
>> Thanks for reducing!
>> If I'm seeing it correctly, the codegen looks fine - the difference is in the IR icmp predicate changing from `slt` to `ult`, and that diff appears to be correct independent of whether or not we use the abs intrinsic:
>> https://alive2.llvm.org/ce/z/VDDqBj
>>
>> So either the source/test has an incorrect expectation and/or something outside of this function is wrong?
>
> It's also correct without the nsw/nuw flags since the inputs are i8 and extended to i32 it can't see i32 INT_MIN.

Just to be sure, i just run the entire compilation of `vc1_block-aarch64.c` through alive2, and as far as i can tell, it did not report any IR-level miscompilations.
So either the original code has bugs/UB's/whatever, or this is an AArch64 backend bug (cc @t.p.northover), or the IR problem is in a place alive2 can't find.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87188