[PATCH] D157598: [InstCombine] Transform sub(sext(add(X, Y)), sext(add(X, Z))) to sub(Y, Z) given nsw adds

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:2474
+  if (match(Op0, m_SExt(m_NSWAdd(m_Value(X), m_Value(Y)))) &&
+      match(Op1, m_SExt(m_NSWAdd(m_Specific(X), m_Value(Z))))) {
+    Value *S1 = Builder.CreateSExt(Y, Ty);
----------------
This assumes the common operand is on the LHS. This will be the case for constants, but may not hold for non-constants. We should handle the commuted case as well.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:2477
+    Value *S2 = Builder.CreateSExt(Z, Ty);
+    return BinaryOperator::CreateSub(S1, S2);
+  }
----------------
It looks like it's legal to transfer the nsw flag to the sub here (https://alive2.llvm.org/ce/z/cYJh4x). But not in the nuw case (https://alive2.llvm.org/ce/z/jHsEcP).


================
Comment at: llvm/test/Transforms/InstCombine/sub-ext-add.ll:4
 
 target datalayout = "e-p:64:64:64-p1:16:16:16-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128"
 
----------------
Drop data layout.


================
Comment at: llvm/test/Transforms/InstCombine/sub-ext-add.ll:13
 ;
   %l3619 = add nuw i8 %a, %C1
   %l3620 = zext i8 %l3619 to i32
----------------
Could you please change the variable names to something more meaningful?


================
Comment at: llvm/test/Transforms/InstCombine/sub-ext-add.ll:92
   ret i32 %l3623
 }
----------------
Please add multi-use tests. I think this fold needs one-use checks.


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

https://reviews.llvm.org/D157598