Sorry, the old title didn't make sense.> Hi, > > I have doubts on the following transformation in InstCombineAddSub.cpp. Is > it always safe to preserve NSW/NUW in this case? > > // If this is a 'B = x-(-A)', change to B = x+A. This preserves NSW/NUW. if (Value <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/include/llvm/IR/Value.h&ct=xref_jump_to_def&cl=GROK&l=69&gsn=Value> * <https://cs.corp.google.com/#piper///depot/google3/GENERATED/figments/cpp/PointerTo/start-with-ll/llvm/class-Value.cc&ct=xref_jump_to_def&cl=GROK&l=3&gsn=*>V <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_usages&gs=cpp:llvm::class-InstCombiner::visitSub(llvm::BinaryOperator%2520&)::V at google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:48204%257Cdef&l=1470&gsn=V> = dyn_castNegVal <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp&ct=xref_jump_to_def&cl=GROK&l=611&gsn=dyn_castNegVal>(Op1 <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_jump_to_def&cl=GROK&l=1456&gsn=Op1>)) { BinaryOperator <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/include/llvm/IR/InstrTypes.h&ct=xref_jump_to_def&cl=GROK&l=138&gsn=BinaryOperator> * <https://cs.corp.google.com/#piper///depot/google3/GENERATED/figments/cpp/PointerTo/start-with-ll/llvm/class-BinaryOperator.cc&ct=xref_jump_to_def&cl=GROK&l=3&gsn=*>Res <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_usages&gs=cpp:llvm::class-InstCombiner::visitSub(llvm::BinaryOperator%2520&)::Res at google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:48251%257Cdef&l=1471&gsn=Res> = BinaryOperator <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/include/llvm/IR/InstrTypes.h&ct=xref_jump_to_def&cl=GROK&l=138&gsn=BinaryOperator>::CreateAdd(Op0 <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_jump_to_def&cl=GROK&l=1456&gsn=Op0>, V <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_jump_to_def&cl=GROK&l=1470&gsn=V>); Res <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_jump_to_def&cl=GROK&l=1471&gsn=Res>->setHasNoSignedWrap <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/IR/Instructions.cpp&ct=xref_jump_to_def&cl=GROK&l=2013&gsn=setHasNoSignedWrap>(I <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_jump_to_def&cl=GROK&l=1455&gsn=I>.hasNoSignedWrap <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/IR/Instructions.cpp&ct=xref_jump_to_def&cl=GROK&l=2025&gsn=hasNoSignedWrap>()); Res <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_jump_to_def&cl=GROK&l=1471&gsn=Res>->setHasNoUnsignedWrap <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/IR/Instructions.cpp&ct=xref_jump_to_def&cl=GROK&l=2009&gsn=setHasNoUnsignedWrap>(I <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_jump_to_def&cl=GROK&l=1455&gsn=I>.hasNoUnsignedWrap <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/IR/Instructions.cpp&ct=xref_jump_to_def&cl=GROK&l=2021&gsn=hasNoUnsignedWrap>()); return Res <https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp&ct=xref_jump_to_def&cl=GROK&l=1471&gsn=Res>; } > > > Suppose A = x = (i4 -8) (the minimum signed integer of i4). The first > minus in x - (-A) does not sign-overflow, but x + A sign-overflows. > > Thanks, > Jingyue >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140624/1c771a7f/attachment.html>
You're right; there's a bug. I proved, however, the correctness of two cases: 1) X - (0 -nuw A) -> X +nuw nsw A 2) X -nsw (0 -nsw A) -> X +nsw A Not sure if the complexity of implementing this is worth the trouble, though. Nuno ----- Original Message ----- From: "Jingyue Wu" <jingyue at google.com> Sent: Wednesday, June 25, 2014 5:48 AM Subject: [LLVMdev] Question Regarding Sign-Overflow> Hi, > > I have doubts on the following transformation in InstCombineAddSub.cpp. Is > it always safe to preserve NSW/NUW in this case? > > // If this is a 'B = x-(-A)', change to B = x+A. This preserves > NSW/NUW. > if (Value *V = dyn_castNegVal(Op1)) { > BinaryOperator *Res = BinaryOperator::CreateAdd(Op0, V); > Res->setHasNoSignedWrap(I.hasNoSignedWrap()); > Res->setHasNoUnsignedWrap(I.hasNoUnsignedWrap()); > return Res; > } > > Suppose A = x = (i4 -8) (the minimum signed integer of i4). The first > minus in x - (-A) does not sign-overflow, but x + A sign-overflows. > > Thanks, > Jingyue
Thanks Nuno! The first case is pretty rare. If (0 - a) does not unsigned overflow, then a must be 0. Right? The second case is worth handling. I'll send a patch soon. Jingyue On Wed, Jun 25, 2014 at 2:00 PM, Nuno Lopes <nunoplopes at sapo.pt> wrote:> You're right; there's a bug. > > I proved, however, the correctness of two cases: > 1) X - (0 -nuw A) -> X +nuw nsw A > 2) X -nsw (0 -nsw A) -> X +nsw A > > Not sure if the complexity of implementing this is worth the trouble, > though. > > Nuno > > ----- Original Message ----- From: "Jingyue Wu" <jingyue at google.com> > Sent: Wednesday, June 25, 2014 5:48 AM > Subject: [LLVMdev] Question Regarding Sign-Overflow > > > > Hi, >> >> I have doubts on the following transformation in InstCombineAddSub.cpp. Is >> it always safe to preserve NSW/NUW in this case? >> >> // If this is a 'B = x-(-A)', change to B = x+A. This preserves >> NSW/NUW. >> if (Value *V = dyn_castNegVal(Op1)) { >> BinaryOperator *Res = BinaryOperator::CreateAdd(Op0, V); >> Res->setHasNoSignedWrap(I.hasNoSignedWrap()); >> Res->setHasNoUnsignedWrap(I.hasNoUnsignedWrap()); >> return Res; >> } >> >> Suppose A = x = (i4 -8) (the minimum signed integer of i4). The first >> minus in x - (-A) does not sign-overflow, but x + A sign-overflows. >> >> Thanks, >> Jingyue >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140625/db528a3b/attachment.html>