[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
llvm-commits at lists.llvm.org
Thu Aug 10 03:41:33 PDT 2023
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
More information about the llvm-commits
mailing list