<div dir="ltr"><a href="https://reviews.llvm.org/rL333610" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>rL333610</a><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 28, 2018 at 6:26 AM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div dir="auto">Thanks. I’m away from a dev machine currently, but I’ll have this fixed by the end of the week, if nobody else gets to it first. IIUC, we can reduce the test to just one IR instruction that includes a constant expression.</div><div><div class="h5"><br><div class="gmail_quote"><div>On Sun, May 27, 2018 at 11:20 PM Mikael Holmén <<a href="mailto:mikael.holmen@ericsson.com" target="_blank">mikael.holmen@ericsson.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I wrote a PR about it:<br>
<a href="https://bugs.llvm.org/show_bug.cgi?id=37605" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_<wbr>bug.cgi?id=37605</a><br>
<br>
Regards,<br>
Mikael<br>
<br>
On 05/25/2018 03:16 PM, Mikael Holmén via llvm-commits wrote:<br>
> Hi Sanjay,<br>
> <br>
> It seems that with this change instcombine can't decide if it prefers <br>
> fadd or fsub and then ping-pongs for a case we found when compiling <br>
> csmith-generated code.<br>
> <br>
> Reproduce with<br>
> opt -S -o - tr16071.ll -instcombine<br>
> <br>
> With -debug we see<br>
> <br>
> INSTCOMBINE ITERATION #1 on test<br>
> IC: DCE: %1 = select i1 %tobool, i1 true, i1 undef<br>
> IC: ADDING: 5 instrs to worklist<br>
> IC: Visiting: %0 = load i16, i16* @c, align 1<br>
> IC: Visiting: %conv = sitofp i16 %0 to float<br>
> IC: Visiting: %sub = fsub float %conv, bitcast (i32 ptrtoint (i16* @b <br>
> to i32) to float)<br>
> IC: Old = %sub = fsub float %conv, bitcast (i32 ptrtoint (i16* @b to <br>
> i32) to float)<br>
> New = <badref> = fadd float %conv, fsub (float -0.000000e+00, <br>
> float bitcast (i32 ptrtoint (i16* @b to i32) to float))<br>
> IC: ADD: %sub = fadd float %conv, fsub (float -0.000000e+00, float <br>
> bitcast (i32 ptrtoint (i16* @b to i32) to float))<br>
> IC: ERASE %1 = fsub float %conv, bitcast (i32 ptrtoint (i16* @b to <br>
> i32) to float)<br>
> IC: ADD: %conv = sitofp i16 %0 to float<br>
> IC: Visiting: %conv = sitofp i16 %0 to float<br>
> IC: Visiting: %sub = fadd float %conv, fsub (float -0.000000e+00, <br>
> float bitcast (i32 ptrtoint (i16* @b to i32) to float))<br>
> IC: Old = %sub = fadd float %conv, fsub (float -0.000000e+00, float <br>
> bitcast (i32 ptrtoint (i16* @b to i32) to float))<br>
> New = <badref> = fsub float %conv, bitcast (i32 ptrtoint (i16* @b <br>
> to i32) to float)<br>
> IC: ADD: %sub = fsub float %conv, bitcast (i32 ptrtoint (i16* @b to <br>
> i32) to float)<br>
> IC: ERASE %1 = fadd float %conv, fsub (float -0.000000e+00, float <br>
> bitcast (i32 ptrtoint (i16* @b to i32) to float))<br>
> IC: ADD: %conv = sitofp i16 %0 to float<br>
> IC: Visiting: %conv = sitofp i16 %0 to float<br>
> IC: Visiting: %sub = fsub float %conv, bitcast (i32 ptrtoint (i16* @b <br>
> to i32) to float)<br>
> IC: Old = %sub = fsub float %conv, bitcast (i32 ptrtoint (i16* @b to <br>
> i32) to float)<br>
> New = <badref> = fadd float %conv, fsub (float -0.000000e+00, <br>
> float bitcast (i32 ptrtoint (i16* @b to i32) to float))<br>
> IC: ADD: %sub = fadd float %conv, fsub (float -0.000000e+00, float <br>
> bitcast (i32 ptrtoint (i16* @b to i32) to float))<br>
> IC: ERASE %1 = fsub float %conv, bitcast (i32 ptrtoint (i16* @b to <br>
> i32) to float)<br>
> IC: ADD: %conv = sitofp i16 %0 to float<br>
> [...]<br>
> <br>
> Regards,<br>
> Mikael<br>
> <br>
> On 04/16/2018 04:13 PM, Sanjay Patel via llvm-commits wrote:<br>
>> Author: spatel<br>
>> Date: Mon Apr 16 07:13:57 2018<br>
>> New Revision: 330126<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=330126&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=330126&view=rev</a><br>
>> Log:<br>
>> [InstCombine] simplify fneg+fadd folds; NFC<br>
>><br>
>> Two cleanups:<br>
>> 1. As noted in D45453, we had tests that don't need FMF that were <br>
>> misplaced in the 'fast-math.ll' test file.<br>
>> 2. This removes the final uses of dyn_castFNegVal, so that can be <br>
>> deleted. We use 'match' now.<br>
>><br>
>> Added:<br>
>> llvm/trunk/test/Transforms/<wbr>InstCombine/fadd.ll<br>
>> Modified:<br>
>> llvm/trunk/lib/Transforms/<wbr>InstCombine/InstCombineAddSub.<wbr>cpp<br>
>> llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstCombineInternal.h<br>
>> llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstructionCombining.cpp<br>
>> llvm/trunk/test/Transforms/<wbr>InstCombine/fast-math.ll<br>
>><br>
>> Modified: llvm/trunk/lib/Transforms/<wbr>InstCombine/InstCombineAddSub.<wbr>cpp<br>
>> URL: <br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp?rev=330126&r1=330125&r2=330126&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/InstCombine/<wbr>InstCombineAddSub.cpp?rev=<wbr>330126&r1=330125&r2=330126&<wbr>view=diff</a> <br>
>><br>
>> ==============================<wbr>==============================<wbr>================== <br>
>><br>
>> --- llvm/trunk/lib/Transforms/<wbr>InstCombine/InstCombineAddSub.<wbr>cpp <br>
>> (original)<br>
>> +++ llvm/trunk/lib/Transforms/<wbr>InstCombine/InstCombineAddSub.<wbr>cpp Mon <br>
>> Apr 16 07:13:57 2018<br>
>> @@ -1308,14 +1308,13 @@ Instruction *InstCombiner::visitFAdd(Bin<br>
>> if (Instruction *FoldedFAdd = foldBinOpIntoSelectOrPhi(I))<br>
>> return FoldedFAdd;<br>
>> - // -A + B --> B - A<br>
>> - if (Value *LHSV = dyn_castFNegVal(LHS))<br>
>> - return BinaryOperator::CreateFSubFMF(<wbr>RHS, LHSV, &I);<br>
>> -<br>
>> - // A + -B --> A - B<br>
>> - if (!isa<Constant>(RHS))<br>
>> - if (Value *V = dyn_castFNegVal(RHS))<br>
>> - return BinaryOperator::CreateFSubFMF(<wbr>LHS, V, &I);<br>
>> + Value *X;<br>
>> + // (-X) + Y --> Y - X<br>
>> + if (match(LHS, m_FNeg(m_Value(X))))<br>
>> + return BinaryOperator::CreateFSubFMF(<wbr>RHS, X, &I);<br>
>> + // Y + (-X) --> Y - X<br>
>> + if (match(RHS, m_FNeg(m_Value(X))))<br>
>> + return BinaryOperator::CreateFSubFMF(<wbr>LHS, X, &I);<br>
>> // Check for (fadd double (sitofp x), y), see if we can merge this <br>
>> into an<br>
>> // integer add followed by a promotion.<br>
>><br>
>> Modified: llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstCombineInternal.h<br>
>> URL: <br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineInternal.h?rev=330126&r1=330125&r2=330126&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/InstCombine/<wbr>InstCombineInternal.h?rev=<wbr>330126&r1=330125&r2=330126&<wbr>view=diff</a> <br>
>><br>
>> ==============================<wbr>==============================<wbr>================== <br>
>><br>
>> --- llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstCombineInternal.h <br>
>> (original)<br>
>> +++ llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstCombineInternal.h Mon <br>
>> Apr 16 07:13:57 2018<br>
>> @@ -376,7 +376,6 @@ private:<br>
>> bool shouldChangeType(unsigned FromBitWidth, unsigned ToBitWidth) <br>
>> const;<br>
>> bool shouldChangeType(Type *From, Type *To) const;<br>
>> Value *dyn_castNegVal(Value *V) const;<br>
>> - Value *dyn_castFNegVal(Value *V, bool NoSignedZero = false) const;<br>
>> Type *FindElementAtOffset(<wbr>PointerType *PtrTy, int64_t Offset,<br>
>> SmallVectorImpl<Value *> &NewIndices);<br>
>><br>
>> Modified: llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstructionCombining.cpp<br>
>> URL: <br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp?rev=330126&r1=330125&r2=330126&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/InstCombine/<wbr>InstructionCombining.cpp?rev=<wbr>330126&r1=330125&r2=330126&<wbr>view=diff</a> <br>
>><br>
>> ==============================<wbr>==============================<wbr>================== <br>
>><br>
>> --- llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstructionCombining.cpp <br>
>> (original)<br>
>> +++ llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstructionCombining.cpp Mon <br>
>> Apr 16 07:13:57 2018<br>
>> @@ -790,23 +790,6 @@ Value *InstCombiner::dyn_castNegVal(<wbr>Valu<br>
>> return nullptr;<br>
>> }<br>
>> -/// Given a 'fsub' instruction, return the RHS of the instruction if <br>
>> the LHS is<br>
>> -/// a constant negative zero (which is the 'negate' form).<br>
>> -Value *InstCombiner::dyn_<wbr>castFNegVal(Value *V, bool IgnoreZeroSign) <br>
>> const {<br>
>> - if (BinaryOperator::isFNeg(V, IgnoreZeroSign))<br>
>> - return BinaryOperator::<wbr>getFNegArgument(V);<br>
>> -<br>
>> - // Constants can be considered to be negated values if they can be <br>
>> folded.<br>
>> - if (ConstantFP *C = dyn_cast<ConstantFP>(V))<br>
>> - return ConstantExpr::getFNeg(C);<br>
>> -<br>
>> - if (ConstantDataVector *C = dyn_cast<ConstantDataVector>(<wbr>V))<br>
>> - if (C->getType()->getElementType(<wbr>)->isFloatingPointTy())<br>
>> - return ConstantExpr::getFNeg(C);<br>
>> -<br>
>> - return nullptr;<br>
>> -}<br>
>> -<br>
>> static Value *<wbr>foldOperationIntoSelectOperand<wbr>(Instruction &I, Value *SO,<br>
>> <wbr> InstCombiner::BuilderTy <br>
>> &Builder) {<br>
>> if (auto *Cast = dyn_cast<CastInst>(&I))<br>
>><br>
>> Added: llvm/trunk/test/Transforms/<wbr>InstCombine/fadd.ll<br>
>> URL: <br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/fadd.ll?rev=330126&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/InstCombine/fadd.<wbr>ll?rev=330126&view=auto</a> <br>
>><br>
>> ==============================<wbr>==============================<wbr>================== <br>
>><br>
>> --- llvm/trunk/test/Transforms/<wbr>InstCombine/fadd.ll (added)<br>
>> +++ llvm/trunk/test/Transforms/<wbr>InstCombine/fadd.ll Mon Apr 16 07:13:57 <br>
>> 2018<br>
>> @@ -0,0 +1,27 @@<br>
>> +; NOTE: Assertions have been autogenerated by <br>
>> utils/update_test_checks.py<br>
>> +; RUN: opt < %s -instcombine -S | FileCheck %s<br>
>> +<br>
>> +; -x + y => y - x<br>
>> +<br>
>> +define float @fneg_op0(float %x, float %y) {<br>
>> +; CHECK-LABEL: @fneg_op0(<br>
>> +; CHECK-NEXT: [[ADD:%.*]] = fsub float [[Y:%.*]], [[X:%.*]]<br>
>> +; CHECK-NEXT: ret float [[ADD]]<br>
>> +;<br>
>> + %neg = fsub float -0.0, %x<br>
>> + %add = fadd float %neg, %y<br>
>> + ret float %add<br>
>> +}<br>
>> +<br>
>> +; x + -y => x - y<br>
>> +<br>
>> +define float @fneg_op1(float %x, float %y) {<br>
>> +; CHECK-LABEL: @fneg_op1(<br>
>> +; CHECK-NEXT: [[ADD:%.*]] = fsub float [[X:%.*]], [[Y:%.*]]<br>
>> +; CHECK-NEXT: ret float [[ADD]]<br>
>> +;<br>
>> + %neg = fsub float -0.0, %y<br>
>> + %add = fadd float %x, %neg<br>
>> + ret float %add<br>
>> +}<br>
>> +<br>
>><br>
>> Modified: llvm/trunk/test/Transforms/<wbr>InstCombine/fast-math.ll<br>
>> URL: <br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/fast-math.ll?rev=330126&r1=330125&r2=330126&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/InstCombine/fast-<wbr>math.ll?rev=330126&r1=330125&<wbr>r2=330126&view=diff</a> <br>
>><br>
>> ==============================<wbr>==============================<wbr>================== <br>
>><br>
>> --- llvm/trunk/test/Transforms/<wbr>InstCombine/fast-math.ll (original)<br>
>> +++ llvm/trunk/test/Transforms/<wbr>InstCombine/fast-math.ll Mon Apr 16 <br>
>> 07:13:57 2018<br>
>> @@ -406,30 +406,6 @@ define float @fold13_reassoc(float %x) {<br>
>> ret float %sub<br>
>> }<br>
>> -; -x + y => y - x<br>
>> -; This is always safe. No FMF required.<br>
>> -define float @fold14(float %x, float %y) {<br>
>> -; CHECK-LABEL: @fold14(<br>
>> -; CHECK-NEXT: [[ADD:%.*]] = fsub float [[Y:%.*]], [[X:%.*]]<br>
>> -; CHECK-NEXT: ret float [[ADD]]<br>
>> -;<br>
>> - %neg = fsub float -0.0, %x<br>
>> - %add = fadd float %neg, %y<br>
>> - ret float %add<br>
>> -}<br>
>> -<br>
>> -; x + -y => x - y<br>
>> -; This is always safe. No FMF required.<br>
>> -define float @fold15(float %x, float %y) {<br>
>> -; CHECK-LABEL: @fold15(<br>
>> -; CHECK-NEXT: [[ADD:%.*]] = fsub float [[X:%.*]], [[Y:%.*]]<br>
>> -; CHECK-NEXT: ret float [[ADD]]<br>
>> -;<br>
>> - %neg = fsub float -0.0, %y<br>
>> - %add = fadd float %x, %neg<br>
>> - ret float %add<br>
>> -}<br>
>> -<br>
>> ; (select X+Y, X-Y) => X + (select Y, -Y)<br>
>> ; This is always safe. No FMF required.<br>
>> define float @fold16(float %x, float %y) {<br>
>><br>
>><br>
>> ______________________________<wbr>_________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
>><br>
> <br>
> <br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
> <br>
<br>
</blockquote></div></div></div></div></blockquote></div><br></div>