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