[PATCH] D48754: [InstCombine] canonicalize abs pattern

[ChenZheng via Phabricator via llvm-commits]

shchenz marked an inline comment as not done.
shchenz added a comment.

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

> Actually, it will fail even without the extra use, so this is a minimal test:
>
>   define i32 @abs_canonical_9(i32 %a, i32 %b) {
>     %tmp2 = sub i32 %b, %a
>     %tmp1 = sub i32 %a, %b
>     %cmp = icmp sgt i32 %tmp1, -1
>     %abs = select i1 %cmp, i32 %tmp1, i32 %tmp2
>     ret i32 %abs
>   }
>  
>
>
> We can not just replace operands without considering the order of the instructions. It would be better to create a new instruction with the IRBuilder, so we don't have to worry about this problem.


@spatel  Hi Sanjay, Sorry about this. Seems this is a big mistake. I will do some study about IRBuilder and upload a new patch later. Thanks for your help.



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4520
     return true;
 
   // Y = sub (0, X)
----------------
spatel wrote:
> There is some white space difference in this file. It should not be included in this patch now.
Yes, I deleted two white spaces added in line 4629 by mistake in rL337143. 


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:836
+
+  // Create the canoical RHS: RHS = sub (0, LHS).
+  if (!match(RHS, m_Neg(m_Specific(LHS)))) {
----------------
spatel wrote:
> typo: canoical
I'll fix that.


https://reviews.llvm.org/D48754