[PATCH] D49238: [InstCombine] add more SPFofSPF folding

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4640-4641
     // Set LHS and RHS so that RHS is the negated operand of the select
+    bool CmpUsesNegatedOp = match(CmpLHS, m_Neg(m_Specific(TrueVal))) ||
+                        match(CmpLHS, m_Neg(m_Specific(FalseVal)));
     if (match(TrueVal, MaybeSExtLHS)) {
----------------
shchenz wrote:
> spatel wrote:
> > What happens if the cmp uses a negated and sign-extended op? I'm not sure if this is possible because of other transforms. Do we have test(s) that include that possibility?
> new patch can recognize:
> 1: 
> %ext = sext i16 %tmp to i32
> %b = sub i32 0, %ext
> %cond = icmp sgt i32 %b, -1
> %ret = select i1 %cond %b, %ext
> 
> %ext can be any operator's result including sext. So I 
> 
> 2: 
> %ext = sext i16 %tmp to i32
> %b = sub i32 0, %ext
> %cond = icmp sgt i32 %ext, -1
> %ret = select i1 %cond %b, %ext
> 
> in this case, it is the same logic with trunk, so I think there should be already test for it. (abs_canonical_5/nabs_canonical_5 in abs-1.ll)
> 
> 3: 
> %b = sub i16 0, %tmp
> %ext = sext i16 %b to i32
> %cond = icmp sgt i32 %ext, -1
> %ret = select i1 %cond %tmp, %X
> 
> for this case:
> if %X is %b, in trunk and new patch, this is not a abs pattern.
> if %X is %ext, in trunk and new patch, this is also not a abs pattern.
> 
> @spatel  Sanjay, Do you have any other pattern I have not considered about? 
> 
> But there should be some thing we can do for sign-extended to enlarge abs pattern scope. for example We should recognize (%tmp, %ext) as negation pair in isKnownNegation(). Maybe I will do this after I finish current work for abs.
The sext pattern is my only concern with this patch. 

Also, I think we might be able to simplify the logic a bit more. I'll send an idea.


https://reviews.llvm.org/D49238