<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>I think this deserves to be raised on llvm-dev as it's own topic.</p>
<p>I am also fine with this staying in while the discussion happens,
but I wouldn't advise building too much on it just yet. I'm okay
with the idea of facts on phis, but I could easily see there being
objections raised.</p>
<p>Philip<br>
</p>
<div class="moz-cite-prefix">On 9/25/19 9:51 AM, Sanjay Patel wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CA+wODiuNSLKYPE0ONS_3eyK721Lo+ktgp2-ia26Aqmd2akLXSQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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"
moz-do-not-send="true">https://bugs.llvm.org/show_bug.cgi?id=38086</a></div>
<a href="https://reviews.llvm.org/D61917"
moz-do-not-send="true">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"
moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">llvm-commits@lists.llvm.org</a><br>
> <a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</blockquote>
</body>
</html>