[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 13:15:25 PDT 2019
I think this deserves to be raised on llvm-dev as it's own topic.
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.
Philip
On 9/25/19 9:51 AM, Sanjay Patel wrote:
> Hi Philip -
>
> I have not thought about the more general possibilities with
> metadata/attributes.
> I don't know of any previous/different discussion of that more general
> case either.
> Should we raise that on llvm-dev and/or revert this?
>
> The FMF-based motivations began here:
> https://bugs.llvm.org/show_bug.cgi?id=38086
> https://reviews.llvm.org/D61917
>
>
> On Wed, Sep 25, 2019 at 11:48 AM Philip Reames
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
> 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 <mailto:llvm-commits at lists.llvm.org>
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190925/443f5acb/attachment.html>
More information about the llvm-commits
mailing list