[llvm] r244928 - Remove and forbid raw_svector_ostream::flush() calls.

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 21 11:44:26 PDT 2015


On Fri, Aug 21, 2015 at 11:42 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015-Aug-13, at 11:12, Yaron Keren via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >
> > Author: yrnkrn
> > Date: Thu Aug 13 13:12:56 2015
> > New Revision: 244928
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=244928&view=rev
> > Log:
> > Remove and forbid raw_svector_ostream::flush() calls.
> > After r244870 flush() will only compare two null pointers and return,
> > doing nothing but wasting run time. The call is not required any more
> > as the stream and its SmallString are always in sync.
> >
> > Thanks to David Blaikie for reviewing.
> >
>
> I wonder about this API.  It seems like more cognitive load for people
> reading the code to have to know whether the stream is auto-flushed or
> not.  Especially if the type definition is a little higher in the
> function.  This also makes it harder to swap in other streams at a
> later time if the needs change.
>
> Alternative suggestion below (I'm not convinced either way, really).
>
> >
> > Modified:
> >    llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileUtils.h
> >    llvm/trunk/include/llvm/Support/raw_ostream.h
> >    llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp
> >    llvm/trunk/lib/MC/MCAsmStreamer.cpp
> >    llvm/trunk/lib/MC/MCAssembler.cpp
> >    llvm/trunk/lib/MC/MCDisassembler/Disassembler.cpp
> >    llvm/trunk/lib/MC/MCELFStreamer.cpp
> >    llvm/trunk/lib/MC/MCMachOStreamer.cpp
> >    llvm/trunk/lib/MC/MCObjectStreamer.cpp
> >    llvm/trunk/lib/MC/WinCOFFStreamer.cpp
> >    llvm/trunk/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
> >    llvm/trunk/lib/Target/TargetMachineC.cpp
> >    llvm/trunk/lib/Target/X86/X86MCInstLower.cpp
> >    llvm/trunk/tools/llvm-objdump/MachODump.cpp
> >    llvm/trunk/utils/TableGen/AsmWriterEmitter.cpp
> >    llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp
> >
> > Modified: llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileUtils.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileUtils.h?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileUtils.h (original)
> > +++ llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileUtils.h Thu Aug
> 13 13:12:56 2015
> > @@ -40,7 +40,6 @@ public:
> >     if (TM.addPassesToEmitMC(PM, Ctx, ObjStream))
> >       llvm_unreachable("Target does not support MC emission.");
> >     PM.run(M);
> > -    ObjStream.flush();
> >     std::unique_ptr<MemoryBuffer> ObjBuffer(
> >         new ObjectMemoryBuffer(std::move(ObjBufferSV)));
> >     ErrorOr<std::unique_ptr<object::ObjectFile>> Obj =
> >
> > Modified: llvm/trunk/include/llvm/Support/raw_ostream.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/raw_ostream.h?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/Support/raw_ostream.h (original)
> > +++ llvm/trunk/include/llvm/Support/raw_ostream.h Thu Aug 13 13:12:56
> 2015
> > @@ -509,6 +509,8 @@ public:
> >   }
> >   ~raw_svector_ostream() override {}
> >
> > +  void flush() = delete;
>
> What about just making this:
>
>     /// No-op, but included for consistency of streaming APIs.
>     void flush() const {}
>
> (and then reverting the rest of the commit...)
>

Seems like code will bitrot pretty quickly to not contain flush calls
because it works OK without them - I'm not sure we'd really get the ability
to safely swap to other streams in the future by having this API...


>
> > +
> >   /// Return a StringRef for the vector contents.
> >   StringRef str() { return StringRef(OS.data(), OS.size()); }
> > };
> >
> > Modified: llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp (original)
> > +++ llvm/trunk/lib/ExecutionEngine/MCJIT/MCJIT.cpp Thu Aug 13 13:12:56
> 2015
> > @@ -159,7 +159,6 @@ std::unique_ptr<MemoryBuffer> MCJIT::emi
> >   // Initialize passes.
> >   PM.run(*M);
> >   // Flush the output buffer to get the generated code into memory
> > -  ObjStream.flush();
> >
> >   std::unique_ptr<MemoryBuffer> CompiledObjBuffer(
> >                                 new
> ObjectMemoryBuffer(std::move(ObjBufferSV)));
> >
> > Modified: llvm/trunk/lib/MC/MCAsmStreamer.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCAsmStreamer.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/MC/MCAsmStreamer.cpp (original)
> > +++ llvm/trunk/lib/MC/MCAsmStreamer.cpp Thu Aug 13 13:12:56 2015
> > @@ -253,9 +253,6 @@ public:
> > void MCAsmStreamer::AddComment(const Twine &T) {
> >   if (!IsVerboseAsm) return;
> >
> > -  // Make sure that CommentStream is flushed.
> > -  CommentStream.flush();
> > -
> >   T.toVector(CommentToEmit);
> >   // Each comment goes on its own line.
> >   CommentToEmit.push_back('\n');
> > @@ -267,7 +264,6 @@ void MCAsmStreamer::EmitCommentsAndEOL()
> >     return;
> >   }
> >
> > -  CommentStream.flush();
> >   StringRef Comments = CommentToEmit;
> >
> >   assert(Comments.back() == '\n' &&
> > @@ -1223,7 +1219,6 @@ void MCAsmStreamer::AddEncodingComment(c
> >   SmallVector<MCFixup, 4> Fixups;
> >   raw_svector_ostream VecOS(Code);
> >   Emitter->encodeInstruction(Inst, VecOS, Fixups, STI);
> > -  VecOS.flush();
> >
> >   // If we are showing fixups, create symbolic markers in the encoded
> >   // representation. We do this by making a per-bit map to the fixup
> item index,
> >
> > Modified: llvm/trunk/lib/MC/MCAssembler.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCAssembler.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/MC/MCAssembler.cpp (original)
> > +++ llvm/trunk/lib/MC/MCAssembler.cpp Thu Aug 13 13:12:56 2015
> > @@ -986,7 +986,6 @@ bool MCAssembler::relaxInstruction(MCAsm
> >   SmallString<256> Code;
> >   raw_svector_ostream VecOS(Code);
> >   getEmitter().encodeInstruction(Relaxed, VecOS, Fixups,
> F.getSubtargetInfo());
> > -  VecOS.flush();
> >
> >   // Update the fragment.
> >   F.setInst(Relaxed);
> > @@ -1009,7 +1008,6 @@ bool MCAssembler::relaxLEB(MCAsmLayout &
> >     encodeSLEB128(Value, OSE);
> >   else
> >     encodeULEB128(Value, OSE);
> > -  OSE.flush();
> >   return OldSize != LF.getContents().size();
> > }
> >
> > @@ -1028,7 +1026,6 @@ bool MCAssembler::relaxDwarfLineAddr(MCA
> >   raw_svector_ostream OSE(Data);
> >   MCDwarfLineAddr::Encode(Context, getDWARFLinetableParams(), LineDelta,
> >                           AddrDelta, OSE);
> > -  OSE.flush();
> >   return OldSize != Data.size();
> > }
> >
> > @@ -1044,7 +1041,6 @@ bool MCAssembler::relaxDwarfCallFrameFra
> >   Data.clear();
> >   raw_svector_ostream OSE(Data);
> >   MCDwarfFrameEmitter::EncodeAdvanceLoc(Context, AddrDelta, OSE);
> > -  OSE.flush();
> >   return OldSize != Data.size();
> > }
> >
> >
> > Modified: llvm/trunk/lib/MC/MCDisassembler/Disassembler.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCDisassembler/Disassembler.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/MC/MCDisassembler/Disassembler.cpp (original)
> > +++ llvm/trunk/lib/MC/MCDisassembler/Disassembler.cpp Thu Aug 13
> 13:12:56 2015
> > @@ -125,7 +125,6 @@ void LLVMDisasmDispose(LLVMDisasmContext
> > static void emitComments(LLVMDisasmContext *DC,
> >                          formatted_raw_ostream &FormattedOS) {
> >   // Flush the stream before taking its content.
> > -  DC->CommentStream.flush();
> >   StringRef Comments = DC->CommentsToEmit.str();
> >   // Get the default information for printing a comment.
> >   const MCAsmInfo *MAI = DC->getAsmInfo();
> > @@ -260,7 +259,6 @@ size_t LLVMDisasmInstruction(LLVMDisasmC
> >     return 0;
> >
> >   case MCDisassembler::Success: {
> > -    Annotations.flush();
> >     StringRef AnnotationsStr = Annotations.str();
> >
> >     SmallVector<char, 64> InsnStr;
> > @@ -272,7 +270,6 @@ size_t LLVMDisasmInstruction(LLVMDisasmC
> >       emitLatency(DC, Inst);
> >
> >     emitComments(DC, FormattedOS);
> > -    OS.flush();
> >
> >     assert(OutStringSize != 0 && "Output buffer cannot be zero size");
> >     size_t OutputSize = std::min(OutStringSize-1, InsnStr.size());
> >
> > Modified: llvm/trunk/lib/MC/MCELFStreamer.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCELFStreamer.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/MC/MCELFStreamer.cpp (original)
> > +++ llvm/trunk/lib/MC/MCELFStreamer.cpp Thu Aug 13 13:12:56 2015
> > @@ -68,7 +68,6 @@ void MCELFStreamer::mergeFragment(MCData
> >       EF->setBundlePadding(static_cast<uint8_t>(RequiredBundlePadding));
> >
> >       Assembler.writeFragmentPadding(*EF, FSize, OW);
> > -      VecOS.flush();
> >       delete OW;
> >
> >       DF->getContents().append(Code.begin(), Code.end());
> > @@ -480,7 +479,6 @@ void MCELFStreamer::EmitInstToData(const
> >   SmallString<256> Code;
> >   raw_svector_ostream VecOS(Code);
> >   Assembler.getEmitter().encodeInstruction(Inst, VecOS, Fixups, STI);
> > -  VecOS.flush();
> >
> >   for (unsigned i = 0, e = Fixups.size(); i != e; ++i)
> >     fixSymbolsInTLSFixups(Fixups[i].getValue());
> >
> > Modified: llvm/trunk/lib/MC/MCMachOStreamer.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCMachOStreamer.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/MC/MCMachOStreamer.cpp (original)
> > +++ llvm/trunk/lib/MC/MCMachOStreamer.cpp Thu Aug 13 13:12:56 2015
> > @@ -443,7 +443,6 @@ void MCMachOStreamer::EmitInstToData(con
> >   SmallString<256> Code;
> >   raw_svector_ostream VecOS(Code);
> >   getAssembler().getEmitter().encodeInstruction(Inst, VecOS, Fixups,
> STI);
> > -  VecOS.flush();
> >
> >   // Add the fixups and data.
> >   for (unsigned i = 0, e = Fixups.size(); i != e; ++i) {
> >
> > Modified: llvm/trunk/lib/MC/MCObjectStreamer.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCObjectStreamer.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/MC/MCObjectStreamer.cpp (original)
> > +++ llvm/trunk/lib/MC/MCObjectStreamer.cpp Thu Aug 13 13:12:56 2015
> > @@ -276,7 +276,6 @@ void MCObjectStreamer::EmitInstToFragmen
> >   raw_svector_ostream VecOS(Code);
> >   getAssembler().getEmitter().encodeInstruction(Inst, VecOS,
> IF->getFixups(),
> >                                                 STI);
> > -  VecOS.flush();
> >   IF->getContents().append(Code.begin(), Code.end());
> > }
> >
> >
> > Modified: llvm/trunk/lib/MC/WinCOFFStreamer.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/WinCOFFStreamer.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/MC/WinCOFFStreamer.cpp (original)
> > +++ llvm/trunk/lib/MC/WinCOFFStreamer.cpp Thu Aug 13 13:12:56 2015
> > @@ -49,7 +49,6 @@ void MCWinCOFFStreamer::EmitInstToData(c
> >   SmallString<256> Code;
> >   raw_svector_ostream VecOS(Code);
> >   getAssembler().getEmitter().encodeInstruction(Inst, VecOS, Fixups,
> STI);
> > -  VecOS.flush();
> >
> >   // Add the fixups and data.
> >   for (unsigned i = 0, e = Fixups.size(); i != e; ++i) {
> > @@ -228,7 +227,6 @@ void MCWinCOFFStreamer::EmitCommonSymbol
> >
> >     OS << " -aligncomm:\"" << Symbol->getName() << "\","
> >        << Log2_32_Ceil(ByteAlignment);
> > -    OS.flush();
> >
> >     PushSection();
> >     SwitchSection(MFI->getDrectveSection());
> >
> > Modified: llvm/trunk/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp (original)
> > +++ llvm/trunk/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp Thu Aug 13
> 13:12:56 2015
> > @@ -136,8 +136,6 @@ void AMDGPUAsmPrinter::EmitInstruction(c
> >       MCCodeEmitter &InstEmitter =
> ObjStreamer.getAssembler().getEmitter();
> >       InstEmitter.encodeInstruction(TmpInst, CodeStream, Fixups,
> >                                     MF->getSubtarget<MCSubtargetInfo>());
> > -      CodeStream.flush();
> > -
> >       HexLines.resize(HexLines.size() + 1);
> >       std::string &HexLine = HexLines.back();
> >       raw_string_ostream HexStream(HexLine);
> >
> > Modified: llvm/trunk/lib/Target/TargetMachineC.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/TargetMachineC.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Target/TargetMachineC.cpp (original)
> > +++ llvm/trunk/lib/Target/TargetMachineC.cpp Thu Aug 13 13:12:56 2015
> > @@ -243,7 +243,6 @@ LLVMBool LLVMTargetMachineEmitToMemoryBu
> >   SmallString<0> CodeString;
> >   raw_svector_ostream OStream(CodeString);
> >   bool Result = LLVMTargetMachineEmit(T, M, OStream, codegen,
> ErrorMessage);
> > -  OStream.flush();
> >
> >   StringRef Data = OStream.str();
> >   *OutMemBuf =
> >
> > Modified: llvm/trunk/lib/Target/X86/X86MCInstLower.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86MCInstLower.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Target/X86/X86MCInstLower.cpp (original)
> > +++ llvm/trunk/lib/Target/X86/X86MCInstLower.cpp Thu Aug 13 13:12:56 2015
> > @@ -92,7 +92,6 @@ namespace llvm {
> >       SmallVector<MCFixup, 4> Fixups;
> >       raw_svector_ostream VecOS(Code);
> >       CodeEmitter->encodeInstruction(Inst, VecOS, Fixups, STI);
> > -      VecOS.flush();
> >       CurrentShadowSize += Code.size();
> >       if (CurrentShadowSize >= RequiredShadowSize)
> >         InShadow = false; // The shadow is big enough. Stop counting.
> >
> > Modified: llvm/trunk/tools/llvm-objdump/MachODump.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/MachODump.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/tools/llvm-objdump/MachODump.cpp (original)
> > +++ llvm/trunk/tools/llvm-objdump/MachODump.cpp Thu Aug 13 13:12:56 2015
> > @@ -5867,7 +5867,6 @@ static void emitComments(raw_svector_ost
> >                          formatted_raw_ostream &FormattedOS,
> >                          const MCAsmInfo &MAI) {
> >   // Flush the stream before taking its content.
> > -  CommentStream.flush();
> >   StringRef Comments = CommentsToEmit.str();
> >   // Get the default information for printing a comment.
> >   const char *CommentBegin = MAI.getCommentString();
> > @@ -6248,7 +6247,6 @@ static void DisassembleMachO(StringRef F
> >             dumpBytes(ArrayRef<uint8_t>(Bytes.data() + Index, Size),
> outs());
> >           }
> >           formatted_raw_ostream FormattedOS(outs());
> > -          Annotations.flush();
> >           StringRef AnnotationsStr = Annotations.str();
> >           if (isThumb)
> >             ThumbIP->printInst(&Inst, FormattedOS, AnnotationsStr,
> *ThumbSTI);
> >
> > Modified: llvm/trunk/utils/TableGen/AsmWriterEmitter.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/AsmWriterEmitter.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/utils/TableGen/AsmWriterEmitter.cpp (original)
> > +++ llvm/trunk/utils/TableGen/AsmWriterEmitter.cpp Thu Aug 13 13:12:56
> 2015
> > @@ -727,7 +727,6 @@ public:
> >         ++I;
> >       }
> >     }
> > -    OS.flush();
> >
> >     // Emit the string.
> >     O.indent(6) << "AsmString = \"" << OutString << "\";\n";
> >
> > Modified: llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp?rev=244928&r1=244927&r2=244928&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp (original)
> > +++ llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp Thu Aug 13
> 13:12:56 2015
> > @@ -1120,7 +1120,6 @@ unsigned FilterChooser::getDecoderIndex(
> >   raw_svector_ostream S(Decoder);
> >   unsigned I = 4;
> >   emitDecoder(S, I, Opc, HasCompleteDecoder);
> > -  S.flush();
> >
> >   // Using the full decoder string as the key value here is a bit
> >   // heavyweight, but is effective. If the string comparisons become a
> > @@ -1231,7 +1230,6 @@ void FilterChooser::emitPredicateTableEn
> >   SmallString<16> PBytes;
> >   raw_svector_ostream S(PBytes);
> >   encodeULEB128(PIdx, S);
> > -  S.flush();
> >
> >   TableInfo.Table.push_back(MCD::OPC_CheckPredicate);
> >   // Predicate index
> > @@ -1297,7 +1295,6 @@ void FilterChooser::emitSoftFailTableEnt
> >   if (NeedNegativeMask) {
> >     MaskBytes.clear();
> >     encodeULEB128(NegativeMask.getZExtValue(), S);
> > -    S.flush();
> >     for (unsigned i = 0, e = MaskBytes.size(); i != e; ++i)
> >       TableInfo.Table.push_back(MaskBytes[i]);
> >   } else
> > @@ -1367,7 +1364,6 @@ void FilterChooser::emitSingletonTableEn
> >   SmallString<16> Bytes;
> >   raw_svector_ostream S(Bytes);
> >   encodeULEB128(DIdx, S);
> > -  S.flush();
> >
> >   // Decoder index
> >   for (unsigned i = 0, e = Bytes.size(); i != e; ++i)
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://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/20150821/2fd44c37/attachment.html>


More information about the llvm-commits mailing list