<div dir="ltr"><div>Hi Philip -</div><div><br></div><div>I have not thought about the more general possibilities with metadata/attributes.</div><div>I don't know of any previous/different discussion of that more general case either.</div><div>Should we raise that on llvm-dev and/or revert this?</div><div><br></div><div>The FMF-based motivations began here:</div><div><div><a href="https://bugs.llvm.org/show_bug.cgi?id=38086">https://bugs.llvm.org/show_bug.cgi?id=38086</a></div><a href="https://reviews.llvm.org/D61917">https://reviews.llvm.org/D61917</a></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Sep 25, 2019 at 11:48 AM Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This seems to be setting a precedent for flags/metadata/attributes on <br>
phi nodes. This is a new concept in the IR, have we discussed the <br>
general implications of this choice? I haven't seen it, but I also may <br>
have simply missed the discussion due to email filtering rules.<br>
<br>
Philip<br>
<br>
On 9/25/19 7:35 AM, Sanjay Patel via llvm-commits wrote:<br>
> Author: spatel<br>
> Date: Wed Sep 25 07:35:02 2019<br>
> New Revision: 372878<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=372878&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=372878&view=rev</a><br>
> Log:<br>
> [IR] allow fast-math-flags on phi of FP values (2nd try)<br>
><br>
> The changes here are based on the corresponding diffs for allowing FMF on 'select':<br>
> D61917 <<a href="https://reviews.llvm.org/D61917" rel="noreferrer" target="_blank">https://reviews.llvm.org/D61917</a>><br>
><br>
> As discussed there, we want to have fast-math-flags be a property of an FP value<br>
> because the alternative (having them on things like fcmp) leads to logical<br>
> inconsistency such as:<br>
> <a href="https://bugs.llvm.org/show_bug.cgi?id=38086" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=38086</a><br>
><br>
> The earlier patch for select made almost no practical difference because most<br>
> unoptimized conditional code begins life as a phi (based on what I see in clang).<br>
> Similarly, I don't expect this patch to do much on its own either because<br>
> SimplifyCFG promptly drops the flags when converting to select on a minimal<br>
> example like:<br>
> <a href="https://bugs.llvm.org/show_bug.cgi?id=39535" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=39535</a><br>
><br>
> But once we have this plumbing in place, we should be able to wire up the FMF<br>
> propagation and start solving cases like that.<br>
><br>
> The change to RecurrenceDescriptor::AddReductionVar() is required to prevent a<br>
> regression in a LoopVectorize test. We are intersecting the FMF of any<br>
> FPMathOperator there, so if a phi is not properly annotated, new math<br>
> instructions may not be either. Once we fix the propagation in SimplifyCFG, it<br>
> may be safe to remove that hack.<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D67564" rel="noreferrer" target="_blank">https://reviews.llvm.org/D67564</a><br>
><br>
> Modified:<br>
> llvm/trunk/docs/LangRef.rst<br>
> llvm/trunk/include/llvm/IR/IRBuilder.h<br>
> llvm/trunk/include/llvm/IR/Operator.h<br>
> llvm/trunk/lib/Analysis/IVDescriptors.cpp<br>
> llvm/trunk/lib/AsmParser/LLParser.cpp<br>
> llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
> llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp<br>
> llvm/trunk/test/Bitcode/compatibility.ll<br>
> llvm/trunk/unittests/IR/InstructionsTest.cpp<br>
><br>
> Modified: llvm/trunk/docs/LangRef.rst<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/docs/LangRef.rst?rev=372878&r1=372877&r2=372878&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/docs/LangRef.rst?rev=372878&r1=372877&r2=372878&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/docs/LangRef.rst (original)<br>
> +++ llvm/trunk/docs/LangRef.rst Wed Sep 25 07:35:02 2019<br>
> @@ -10067,7 +10067,7 @@ Syntax:<br>
> <br>
> ::<br>
> <br>
> - <result> = phi <ty> [ <val0>, <label0>], ...<br>
> + <result> = phi [fast-math-flags] <ty> [ <val0>, <label0>], ...<br>
> <br>
> Overview:<br>
> """""""""<br>
> @@ -10094,6 +10094,12 @@ deemed to occur on the edge from the cor<br>
> the current block (but after any definition of an '``invoke``'<br>
> instruction's return value on the same edge).<br>
> <br>
> +The optional ``fast-math-flags`` marker indicates that the phi has one<br>
> +or more :ref:`fast-math-flags <fastmath>`. These are optimization hints<br>
> +to enable otherwise unsafe floating-point optimizations. Fast-math-flags<br>
> +are only valid for phis that return a floating-point scalar or vector<br>
> +type.<br>
> +<br>
> Semantics:<br>
> """"""""""<br>
> <br>
><br>
> Modified: llvm/trunk/include/llvm/IR/IRBuilder.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/IRBuilder.h?rev=372878&r1=372877&r2=372878&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/IRBuilder.h?rev=372878&r1=372877&r2=372878&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/IR/IRBuilder.h (original)<br>
> +++ llvm/trunk/include/llvm/IR/IRBuilder.h Wed Sep 25 07:35:02 2019<br>
> @@ -2231,7 +2231,10 @@ public:<br>
> <br>
> PHINode *CreatePHI(Type *Ty, unsigned NumReservedValues,<br>
> const Twine &Name = "") {<br>
> - return Insert(PHINode::Create(Ty, NumReservedValues), Name);<br>
> + PHINode *Phi = PHINode::Create(Ty, NumReservedValues);<br>
> + if (isa<FPMathOperator>(Phi))<br>
> + Phi = cast<PHINode>(setFPAttrs(Phi, nullptr /* MDNode* */, FMF));<br>
> + return Insert(Phi, Name);<br>
> }<br>
> <br>
> CallInst *CreateCall(FunctionType *FTy, Value *Callee,<br>
><br>
> Modified: llvm/trunk/include/llvm/IR/Operator.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Operator.h?rev=372878&r1=372877&r2=372878&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Operator.h?rev=372878&r1=372877&r2=372878&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/IR/Operator.h (original)<br>
> +++ llvm/trunk/include/llvm/IR/Operator.h Wed Sep 25 07:35:02 2019<br>
> @@ -379,13 +379,17 @@ public:<br>
> return false;<br>
> <br>
> switch (Opcode) {<br>
> + // FIXME: To clean up and correct the semantics of fast-math-flags, FCmp<br>
> + // should not be treated as a math op, but the other opcodes should.<br>
> + // This would make things consistent with Select/PHI (FP value type<br>
> + // determines whether they are math ops and, therefore, capable of<br>
> + // having fast-math-flags).<br>
> case Instruction::FCmp:<br>
> return true;<br>
> // non math FP Operators (no FMF)<br>
> case Instruction::ExtractElement:<br>
> case Instruction::ShuffleVector:<br>
> case Instruction::InsertElement:<br>
> - case Instruction::PHI:<br>
> return false;<br>
> default:<br>
> return V->getType()->isFPOrFPVectorTy();<br>
><br>
> Modified: llvm/trunk/lib/Analysis/IVDescriptors.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/IVDescriptors.cpp?rev=372878&r1=372877&r2=372878&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/IVDescriptors.cpp?rev=372878&r1=372877&r2=372878&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Analysis/IVDescriptors.cpp (original)<br>
> +++ llvm/trunk/lib/Analysis/IVDescriptors.cpp Wed Sep 25 07:35:02 2019<br>
> @@ -300,7 +300,8 @@ bool RecurrenceDescriptor::AddReductionV<br>
> ReduxDesc = isRecurrenceInstr(Cur, Kind, ReduxDesc, HasFunNoNaNAttr);<br>
> if (!ReduxDesc.isRecurrence())<br>
> return false;<br>
> - if (isa<FPMathOperator>(ReduxDesc.getPatternInst()))<br>
> + // FIXME: FMF is allowed on phi, but propagation is not handled correctly.<br>
> + if (isa<FPMathOperator>(ReduxDesc.getPatternInst()) && !IsAPhi)<br>
> FMF &= ReduxDesc.getPatternInst()->getFastMathFlags();<br>
> }<br>
> <br>
><br>
> Modified: llvm/trunk/lib/AsmParser/LLParser.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/AsmParser/LLParser.cpp?rev=372878&r1=372877&r2=372878&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/AsmParser/LLParser.cpp?rev=372878&r1=372877&r2=372878&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/AsmParser/LLParser.cpp (original)<br>
> +++ llvm/trunk/lib/AsmParser/LLParser.cpp Wed Sep 25 07:35:02 2019<br>
> @@ -5802,7 +5802,19 @@ int LLParser::ParseInstruction(Instructi<br>
> case lltok::kw_extractelement: return ParseExtractElement(Inst, PFS);<br>
> case lltok::kw_insertelement: return ParseInsertElement(Inst, PFS);<br>
> case lltok::kw_shufflevector: return ParseShuffleVector(Inst, PFS);<br>
> - case lltok::kw_phi: return ParsePHI(Inst, PFS);<br>
> + case lltok::kw_phi: {<br>
> + FastMathFlags FMF = EatFastMathFlagsIfPresent();<br>
> + int Res = ParsePHI(Inst, PFS);<br>
> + if (Res != 0)<br>
> + return Res;<br>
> + if (FMF.any()) {<br>
> + if (!Inst->getType()->isFPOrFPVectorTy())<br>
> + return Error(Loc, "fast-math-flags specified for phi without "<br>
> + "floating-point scalar or vector return type");<br>
> + Inst->setFastMathFlags(FMF);<br>
> + }<br>
> + return 0;<br>
> + }<br>
> case lltok::kw_landingpad: return ParseLandingPad(Inst, PFS);<br>
> // Call.<br>
> case lltok::kw_call: return ParseCall(Inst, PFS, CallInst::TCK_None);<br>
><br>
> Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=372878&r1=372877&r2=372878&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=372878&r1=372877&r2=372878&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)<br>
> +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Wed Sep 25 07:35:02 2019<br>
> @@ -4629,31 +4629,48 @@ Error BitcodeReader::parseFunctionBody(F<br>
> InstructionList.push_back(I);<br>
> break;<br>
> case bitc::FUNC_CODE_INST_PHI: { // PHI: [ty, val0,bb0, ...]<br>
> - if (Record.size() < 1 || ((Record.size()-1)&1))<br>
> + if (Record.size() < 1)<br>
> return error("Invalid record");<br>
> + // The first record specifies the type.<br>
> FullTy = getFullyStructuredTypeByID(Record[0]);<br>
> Type *Ty = flattenPointerTypes(FullTy);<br>
> if (!Ty)<br>
> return error("Invalid record");<br>
> <br>
> - PHINode *PN = PHINode::Create(Ty, (Record.size()-1)/2);<br>
> + // Phi arguments are pairs of records of [value, basic block].<br>
> + // There is an optional final record for fast-math-flags if this phi has a<br>
> + // floating-point type.<br>
> + size_t NumArgs = (Record.size() - 1) / 2;<br>
> + if ((Record.size() - 1) % 2 == 1 && !Ty->isFloatingPointTy())<br>
> + return error("Invalid record");<br>
> +<br>
> + PHINode *PN = PHINode::Create(Ty, NumArgs);<br>
> InstructionList.push_back(PN);<br>
> <br>
> - for (unsigned i = 0, e = Record.size()-1; i != e; i += 2) {<br>
> + for (unsigned i = 0; i != NumArgs; i++) {<br>
> Value *V;<br>
> // With the new function encoding, it is possible that operands have<br>
> // negative IDs (for forward references). Use a signed VBR<br>
> // representation to keep the encoding small.<br>
> if (UseRelativeIDs)<br>
> - V = getValueSigned(Record, 1+i, NextValueNo, Ty);<br>
> + V = getValueSigned(Record, i * 2 + 1, NextValueNo, Ty);<br>
> else<br>
> - V = getValue(Record, 1+i, NextValueNo, Ty);<br>
> - BasicBlock *BB = getBasicBlock(Record[2+i]);<br>
> + V = getValue(Record, i * 2 + 1, NextValueNo, Ty);<br>
> + BasicBlock *BB = getBasicBlock(Record[i * 2 + 2]);<br>
> if (!V || !BB)<br>
> return error("Invalid record");<br>
> PN->addIncoming(V, BB);<br>
> }<br>
> I = PN;<br>
> +<br>
> + // If there are an even number of records, the final record must be FMF.<br>
> + if (Record.size() % 2 == 0) {<br>
> + assert(isa<FPMathOperator>(I) && "Unexpected phi type");<br>
> + FastMathFlags FMF = getDecodedFastMathFlags(Record[Record.size() - 1]);<br>
> + if (FMF.any())<br>
> + I->setFastMathFlags(FMF);<br>
> + }<br>
> +<br>
> break;<br>
> }<br>
> <br>
><br>
> Modified: llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp?rev=372878&r1=372877&r2=372878&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp?rev=372878&r1=372877&r2=372878&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp (original)<br>
> +++ llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp Wed Sep 25 07:35:02 2019<br>
> @@ -2880,6 +2880,11 @@ void ModuleBitcodeWriter::writeInstructi<br>
> pushValueSigned(PN.getIncomingValue(i), InstID, Vals64);<br>
> Vals64.push_back(VE.getValueID(PN.getIncomingBlock(i)));<br>
> }<br>
> +<br>
> + uint64_t Flags = getOptimizationFlags(&I);<br>
> + if (Flags != 0)<br>
> + Vals64.push_back(Flags);<br>
> +<br>
> // Emit a Vals64 vector and exit.<br>
> Stream.EmitRecord(Code, Vals64, AbbrevToUse);<br>
> Vals64.clear();<br>
><br>
> Modified: llvm/trunk/test/Bitcode/compatibility.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Bitcode/compatibility.ll?rev=372878&r1=372877&r2=372878&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Bitcode/compatibility.ll?rev=372878&r1=372877&r2=372878&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/Bitcode/compatibility.ll (original)<br>
> +++ llvm/trunk/test/Bitcode/compatibility.ll Wed Sep 25 07:35:02 2019<br>
> @@ -861,6 +861,27 @@ define void @fastmathflags_vector_select<br>
> ret void<br>
> }<br>
> <br>
> +define void @fastmathflags_phi(i1 %cond, float %f1, float %f2, double %d1, double %d2, half %h1, half %h2) {<br>
> +entry:<br>
> + br i1 %cond, label %L1, label %L2<br>
> +L1:<br>
> + br label %exit<br>
> +L2:<br>
> + br label %exit<br>
> +exit:<br>
> + %p.nnan = phi nnan float [ %f1, %L1 ], [ %f2, %L2 ]<br>
> + ; CHECK: %p.nnan = phi nnan float [ %f1, %L1 ], [ %f2, %L2 ]<br>
> + %p.ninf = phi ninf double [ %d1, %L1 ], [ %d2, %L2 ]<br>
> + ; CHECK: %p.ninf = phi ninf double [ %d1, %L1 ], [ %d2, %L2 ]<br>
> + %p.contract = phi contract half [ %h1, %L1 ], [ %h2, %L2 ]<br>
> + ; CHECK: %p.contract = phi contract half [ %h1, %L1 ], [ %h2, %L2 ]<br>
> + %p.nsz.reassoc = phi reassoc nsz float [ %f1, %L1 ], [ %f2, %L2 ]<br>
> + ; CHECK: %p.nsz.reassoc = phi reassoc nsz float [ %f1, %L1 ], [ %f2, %L2 ]<br>
> + %p.fast = phi fast half [ %h2, %L1 ], [ %h1, %L2 ]<br>
> + ; CHECK: %p.fast = phi fast half [ %h2, %L1 ], [ %h1, %L2 ]<br>
> + ret void<br>
> +}<br>
> +<br>
> ; Check various fast math flags and floating-point types on calls.<br>
> <br>
> declare float @fmf1()<br>
><br>
> Modified: llvm/trunk/unittests/IR/InstructionsTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/InstructionsTest.cpp?rev=372878&r1=372877&r2=372878&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/InstructionsTest.cpp?rev=372878&r1=372877&r2=372878&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/unittests/IR/InstructionsTest.cpp (original)<br>
> +++ llvm/trunk/unittests/IR/InstructionsTest.cpp Wed Sep 25 07:35:02 2019<br>
> @@ -1034,13 +1034,16 @@ TEST(InstructionsTest, SkipDebug) {<br>
> EXPECT_EQ(nullptr, Term->getNextNonDebugInstruction());<br>
> }<br>
> <br>
> -TEST(InstructionsTest, PhiIsNotFPMathOperator) {<br>
> +TEST(InstructionsTest, PhiMightNotBeFPMathOperator) {<br>
> LLVMContext Context;<br>
> IRBuilder<> Builder(Context);<br>
> MDBuilder MDHelper(Context);<br>
> - Instruction *I = Builder.CreatePHI(Builder.getDoubleTy(), 0);<br>
> + Instruction *I = Builder.CreatePHI(Builder.getInt32Ty(), 0);<br>
> EXPECT_FALSE(isa<FPMathOperator>(I));<br>
> I->deleteValue();<br>
> + Instruction *FP = Builder.CreatePHI(Builder.getDoubleTy(), 0);<br>
> + EXPECT_TRUE(isa<FPMathOperator>(FP));<br>
> + FP->deleteValue();<br>
> }<br>
> <br>
> TEST(InstructionsTest, FNegInstruction) {<br>
><br>
><br>
> _______________________________________________<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="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>