[PATCH] D90734: [EarlyCSE] make abs recognization not depend on instcombine abs canonicalize

Tue Nov 10 06:23:05 PST 2020

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

In D90734#2385581 <https://reviews.llvm.org/D90734#2385581>, @shchenz wrote:

> In D90734#2385565 <https://reviews.llvm.org/D90734#2385565>, @shchenz wrote:
>
>> I am thinking `llvm.abs(A)` and `llvm.abs(-A)` can not be optimized in current earlyCSE phase? There are logics for commutative intrinsics and intrinsics with same operands in EarlyCSE pass. Am I missing something?
>
> Ah, I know your ideas ^-^. Instcombine should canonicalize `llvm.abs(-A)` to `llvm.abs(A)`. So after instcombine, in earlycse, we will not see `llvm.abs(-A)` right? I don't have other concerns. Thanks.

Right - I do not think we should put any extra pattern matching logic into this pass unless it is very simple. Everything else is dangerous - as this bug/patch (and a few before it) have already shown. :)
LGTM.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90734