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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D87188#2281093 <https://reviews.llvm.org/D87188#2281093>, @mstorsjo wrote:

> This broke a few tests for me (generating code that now gives the fail result at runtime).
>
> I'm not entirely sure which bit is the culprit, but the difference in output (that breaks tests) for one object file is available at https://martin.st/temp/vc1_block-diff-aarch64.txt, and https://martin.st/temp/vc1_block-diff-armv7.txt. For the aarch64 version, it looks like some conditionals are inverted, like these changes:
>
>   -	b.ge	.LBB1_124
>   +	b.hs	.LBB1_124
>
> and
>
>   -	csel	w14, w15, w14, gt
>   +	csel	w14, w15, w14, hi
>
> (with no seemingly related changes that would change the roles of the registers).
>
> The input files for reproducing the issues are available at https://martin.st/temp/vc1_block-aarch64.c and https://martin.st/temp/vc1_block-armv7.c, which can be compiled with `clang -target {aarch64,armv7}-w64-mingw32 -c -O2 vc1_block-{aarch64,armv7}.c`.

Based on an initial look, the changes in comparison predicates here are probably a red herring. If I understand right, those predicates are switching from signed to unsigned comparison (e.g. gt to hi). I also see similar changes in IR. However, all the cases I looked at are actually correct (e.g. abs(x) > abs(y) can be compared signed or unsigned for poisoning abs). Assuming that this code is clean under ubsan, the problem is likely something else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87188