[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc (WIP)

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1070
                                         InstCombinerImpl &IC) {
   if (!Cmp.hasOneUse() || !isa<Constant>(Cmp.getOperand(1)))
     return nullptr;
----------------
I'm wondering what to do about this one use check. Should we canonicalize even if the compare has other uses?

We are folding cmp+sub+select into abs or abs+sub. However, both cmp and sub can potentially have extra uses.


================
Comment at: llvm/test/Transforms/InstCombine/abs_abs.ll:94
+; CHECK-NEXT:    [[COND18:%.*]] = select i1 [[CMP1_NOT]], i32 [[SUB16]], i32 [[TMP1]]
+; CHECK-NEXT:    ret i32 [[COND18]]
 ;
----------------
This is the main remaining problem: What happens here is that we see `abs(x) > 0`, convert this to `abs(x) != 0` (based on abs range) and then to `x != 0`. The last step happens due to the fold I added in rGada8a17d945c17c5603e24824f642ca199412adf, previously this worked. At that point we are left with something like `x != 0 ? abs(x) : -abs(x)`, which is no longer a proper abs.

What do you think the best way to handle this would be? Should this pattern be added to the SPF_ABS recognition? Just explicitly match it in InstCombine?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87188