[PATCH] D48754: recognize more abs pattern

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

In https://reviews.llvm.org/D48754#1159009, @spatel wrote:

> In https://reviews.llvm.org/D48754#1158271, @shchenz wrote:
>
> > @spatel  Hi Sanjay, I have made changes accordingly. Could you help to have another review. Thanks.
>
>
> Thanks for the changes. This looks mostly right, but I want to make sure we're testing everything properly. This is really several independent steps in 1 patch, so I'd like to reduce the risk of bugs by breaking it up.
>
> 1. Please separate the creation of isKnownNegation() into its own patch and use it from InstSimplify -> SimplifyAddInst(). I have added tests at https://reviews.llvm.org/rL336822, so that should be a very small patch. Improving InstSimplify helps several other passes, so that's a good patch independent of abs().
> 2. The changes in matchSelectPattern can be separated and tested using patterns that are optimized by InstCombiner::foldSPFofSPF(). We'll need to add some tests for that part. Let me know if it is not clear.
> 3. The canonicalization changes will be the last step and should be covered by the tests shown in this patch, but I think we're missing some cases where the code may have bugs.


@spatel Hi Sanjay, I have made patches for 1 & 2. But I am not very clear about 3. You mentioned the code may have bugs, could you give me some details? I saw your inline comment "What happens if the subtract has other uses", do you mean this one? In my opinion, RHS from matchSelectPattern() currently must be sub(0, X) or sub(A, B) (LHS = sub(B, A)). If I change RHS(TVal or FVal) to sub(0, LHS), I think there will be no impact for RHS(TVal or FVal) other users. yes, you are right, there should be some test cases for this scenario. Thanks.


https://reviews.llvm.org/D48754