[llvm] r222935 - Simplify some more ownership using forward_list<T> rather than vector<unique_ptr<T>>
David Blaikie
dblaikie at gmail.com
Fri Dec 19 14:11:13 PST 2014
On Wed, Dec 3, 2014 at 3:57 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> I looked into test/CodeGen/X86/compact-unwind.ll. llvm-mc was giving
> different results. Given that you changed `tablegen`, I frankly gave
> up on figuring out exactly why.
>
> I went back and reviewed the patch for changed semantics, and took a
> shot in the dark. The `sort()` at the end is different from the
> old `sort()`, because items have been front-loaded instead of back-
> loaded.
>
> The fix is to sort it backwards and then reverse the list. This passes
> `check` for me.
>
Any preferences on this approach, versus using std::list (paying an extra
pointer per element) and pushing onto the end instead? (hmm, or maybe we
can keep an iterator to the tail end of the list and just use that as an
insert hint while still using std::forward_list... - not sure if that's
worth it)
Also - did you happen to verify that your change causes the diffs on the
AsmMatcher.inc files you mentioned to go away entirely, or were there other
remaining changes I should hunt for/verify?
>
> I don't know why this is platform-specific... I can't think of why
> that makes sense.
>
Yeah, that's still rather confusing.
>
>
>
> > On 2014-Dec-03, at 14:39, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Fri, Nov 28, 2014 at 3:04 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > I reverted this (and the follow-up in r222938) in r222943.
> >
> > Skimming the commit, I suspect the failures are caused by front-loading
> > the sequence instead of back-loading, but I didn't have a chance to
> > investigate.
> >
> > Let me know if the failures don't reproduce for you and I can help you
> > dig into it.
> >
> > Wouldn't mind a little help here - some of the tests failing (just
> taking the first one as a random example,
> test/CodeGen/X86/compact-unwind.ll) have hardcoded triples for darwin, so
> it doesn't seem like it's a matter of the target, so much as the host - I
> don't have a build setup on macos, so I'm not sure if there's anything more
> I can do to reproduce these failures...
> >
> > - David
> >
> >
> > > On 2014 Nov 28, at 14:38, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > >
> > > Our public incremental builder started failing with this commit:
> > >
> http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-incremental/1391/
> > >
> > > Previous build was r222933, and was clean:
> > >
> http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-incremental/1390/
> > >
> > > (r222934 was formatting-only.)
> > >
> > > Our non-incremental builder started failing around then as well:
> > > http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA/1251/
> > >
> > > Here's the failure:
> > >
> > > Failing Tests (34):
> > > LLVM :: CodeGen/X86/compact-unwind.ll
> > > LLVM :: CodeGen/X86/no-compact-unwind.ll
> > > LLVM :: ExecutionEngine/RuntimeDyld/X86/MachO_i386_eh_frame.s
> > > LLVM :: MC/ARM/basic-arm-instructions.s
> > > LLVM :: MC/ARM/basic-thumb-instructions.s
> > > LLVM :: MC/ARM/thumb-diagnostics.s
> > > LLVM :: MC/ARM/v8_IT_manual.s
> > > LLVM :: MC/COFF/seh-align3.s
> > > LLVM :: MC/COFF/seh-section.s
> > > LLVM :: MC/COFF/seh.s
> > > LLVM :: MC/ELF/relax-all-flag.s
> > > LLVM :: MC/ELF/relax.s
> > > LLVM :: MC/ELF/subsection.s
> > > LLVM :: MC/MachO/ARM/llvm-objdump-macho.s
> > > LLVM :: MC/MachO/ARM/long-call-branch-island-relocation.s
> > > LLVM :: MC/MachO/darwin-x86_64-diff-relocs.s
> > > LLVM :: MC/MachO/darwin-x86_64-nobase-relocs.s
> > > LLVM :: MC/MachO/direction_labels.s
> > > LLVM :: MC/MachO/jcc.s
> > > LLVM :: MC/MachO/relax-recompute-align.s
> > > LLVM :: MC/MachO/reloc.s
> > > LLVM :: MC/Mips/micromips-16-bit-instructions.s
> > > LLVM :: MC/Mips/micromips-control-instructions.s
> > > LLVM :: MC/X86/AlignedBundling/long-nop-pad.s
> > > LLVM :: MC/X86/AlignedBundling/relax-at-bundle-end.s
> > > LLVM :: MC/X86/AlignedBundling/relax-in-bundle-group.s
> > > LLVM :: MC/X86/AlignedBundling/single-inst-bundling.s
> > > LLVM :: MC/X86/avx512-encodings.s
> > > LLVM :: MC/X86/intel-syntax-encoding.s
> > > LLVM :: MC/X86/stackmap-nops.ll
> > > LLVM :: MC/X86/x86-64-avx512bw.s
> > > LLVM :: MC/X86/x86-64-avx512bw_vl.s
> > > LLVM :: MC/X86/x86-64-avx512f_vl.s
> > > LLVM :: MC/X86/x86-64.s
> > >
> > > Let me know if you need help reproducing.
> > >
> > >> On 2014 Nov 28, at 13:20, David Blaikie <dblaikie at gmail.com> wrote:
> > >>
> > >> Author: dblaikie
> > >> Date: Fri Nov 28 15:20:24 2014
> > >> New Revision: 222935
> > >>
> > >> URL: http://llvm.org/viewvc/llvm-project?rev=222935&view=rev
> > >> Log:
> > >> Simplify some more ownership using forward_list<T> rather than
> vector<unique_ptr<T>>
> > >>
> > >> Modified:
> > >> llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp
> > >>
> > >> Modified: llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp
> > >> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp?rev=222935&r1=222934&r2=222935&view=diff
> > >>
> ==============================================================================
> > >> --- llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp (original)
> > >> +++ llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp Fri Nov 28
> 15:20:24 2014
> > >> @@ -616,7 +616,7 @@ public:
> > >> std::forward_list<ClassInfo> Classes;
> > >>
> > >> /// The information on the matchables to match.
> > >> - std::vector<std::unique_ptr<MatchableInfo>> Matchables;
> > >> + std::forward_list<MatchableInfo> Matchables;
> > >>
> > >> /// Info for custom matching operands by user defined methods.
> > >> std::vector<OperandMatchEntry> OperandMatchInfo;
> > >> @@ -1285,8 +1285,8 @@ void AsmMatcherInfo::buildOperandMatchIn
> > >>
> > >> // Keep track of all operands of this instructions which belong to
> the
> > >> // same class.
> > >> - for (unsigned i = 0, e = MI->AsmOperands.size(); i != e; ++i) {
> > >> - const MatchableInfo::AsmOperand &Op = MI->AsmOperands[i];
> > >> + for (unsigned i = 0, e = MI.AsmOperands.size(); i != e; ++i) {
> > >> + const MatchableInfo::AsmOperand &Op = MI.AsmOperands[i];
> > >> if (Op.Class->ParserMethod.empty())
> > >> continue;
> > >> unsigned &OperandMask = OpClassMask[Op.Class];
> > >> @@ -1297,8 +1297,7 @@ void AsmMatcherInfo::buildOperandMatchIn
> > >> for (const auto &OCM : OpClassMask) {
> > >> unsigned OpMask = OCM.second;
> > >> ClassInfo *CI = OCM.first;
> > >> - OperandMatchInfo.push_back(OperandMatchEntry::create(MI.get(),
> CI,
> > >> - OpMask));
> > >> + OperandMatchInfo.push_back(OperandMatchEntry::create(&MI, CI,
> OpMask));
> > >> }
> > >> }
> > >> }
> > >> @@ -1345,16 +1344,15 @@ void AsmMatcherInfo::buildInfo() {
> > >> if (CGI->TheDef->getValueAsBit("isCodeGenOnly"))
> > >> continue;
> > >>
> > >> - std::unique_ptr<MatchableInfo> II(new MatchableInfo(*CGI));
> > >> + Matchables.emplace_front(*CGI);
> > >> + MatchableInfo *II = &Matchables.front();
> > >>
> > >> II->initialize(*this, SingletonRegisters, AsmVariantNo,
> RegisterPrefix);
> > >>
> > >> // Ignore instructions which shouldn't be matched and diagnose
> invalid
> > >> // instruction definitions with an error.
> > >> if (!II->validate(CommentDelimiter, true))
> > >> - continue;
> > >> -
> > >> - Matchables.push_back(std::move(II));
> > >> + Matchables.pop_front();
> > >> }
> > >>
> > >> // Parse all of the InstAlias definitions and stick them in the
> list of
> > >> @@ -1372,14 +1370,13 @@ void AsmMatcherInfo::buildInfo() {
> > >> .startswith( MatchPrefix))
> > >> continue;
> > >>
> > >> - std::unique_ptr<MatchableInfo> II(new MatchableInfo(Alias));
> > >> + Matchables.emplace_front(Alias);
> > >> + MatchableInfo *II = &Matchables.front();
> > >>
> > >> II->initialize(*this, SingletonRegisters, AsmVariantNo,
> RegisterPrefix);
> > >>
> > >> // Validate the alias definitions.
> > >> II->validate(CommentDelimiter, false);
> > >> -
> > >> - Matchables.push_back(std::move(II));
> > >> }
> > >> }
> > >>
> > >> @@ -1391,17 +1388,17 @@ void AsmMatcherInfo::buildInfo() {
> > >>
> > >> // Build the information about matchables, now that we have fully
> formed
> > >> // classes.
> > >> - std::vector<std::unique_ptr<MatchableInfo>> NewMatchables;
> > >> + std::forward_list<MatchableInfo> NewMatchables;
> > >> for (auto &II : Matchables) {
> > >> // Parse the tokens after the mnemonic.
> > >> // Note: buildInstructionOperandReference may insert new
> AsmOperands, so
> > >> // don't precompute the loop bound.
> > >> - for (unsigned i = 0; i != II->AsmOperands.size(); ++i) {
> > >> - MatchableInfo::AsmOperand &Op = II->AsmOperands[i];
> > >> + for (unsigned i = 0; i != II.AsmOperands.size(); ++i) {
> > >> + MatchableInfo::AsmOperand &Op = II.AsmOperands[i];
> > >> StringRef Token = Op.Token;
> > >>
> > >> // Check for singleton registers.
> > >> - if (Record *RegRecord = II->AsmOperands[i].SingletonReg) {
> > >> + if (Record *RegRecord = II.AsmOperands[i].SingletonReg) {
> > >> Op.Class = RegisterClasses[RegRecord];
> > >> assert(Op.Class && Op.Class->Registers.size() == 1 &&
> > >> "Unexpected class for singleton register");
> > >> @@ -1426,35 +1423,30 @@ void AsmMatcherInfo::buildInfo() {
> > >> else
> > >> OperandName = Token.substr(1);
> > >>
> > >> - if (II->DefRec.is<const CodeGenInstruction*>())
> > >> - buildInstructionOperandReference(II.get(), OperandName, i);
> > >> + if (II.DefRec.is<const CodeGenInstruction *>())
> > >> + buildInstructionOperandReference(&II, OperandName, i);
> > >> else
> > >> - buildAliasOperandReference(II.get(), OperandName, Op);
> > >> + buildAliasOperandReference(&II, OperandName, Op);
> > >> }
> > >>
> > >> - if (II->DefRec.is<const CodeGenInstruction*>()) {
> > >> - II->buildInstructionResultOperands();
> > >> + if (II.DefRec.is<const CodeGenInstruction *>()) {
> > >> + II.buildInstructionResultOperands();
> > >> // If the instruction has a two-operand alias, build up the
> > >> // matchable here. We'll add them in bulk at the end to avoid
> > >> // confusing this loop.
> > >> std::string Constraint =
> > >> - II->TheDef->getValueAsString("TwoOperandAliasConstraint");
> > >> + II.TheDef->getValueAsString("TwoOperandAliasConstraint");
> > >> if (Constraint != "") {
> > >> // Start by making a copy of the original matchable.
> > >> - std::unique_ptr<MatchableInfo> AliasII(new
> MatchableInfo(*II));
> > >> + NewMatchables.emplace_front(II);
> > >>
> > >> // Adjust it to be a two-operand alias.
> > >> - AliasII->formTwoOperandAlias(Constraint);
> > >> -
> > >> - // Add the alias to the matchables list.
> > >> - NewMatchables.push_back(std::move(AliasII));
> > >> + NewMatchables.front().formTwoOperandAlias(Constraint);
> > >> }
> > >> } else
> > >> - II->buildAliasResultOperands();
> > >> + II.buildAliasResultOperands();
> > >> }
> > >> - if (!NewMatchables.empty())
> > >> - std::move(NewMatchables.begin(), NewMatchables.end(),
> > >> - std::back_inserter(Matchables));
> > >> + Matchables.splice_after(Matchables.before_begin(), NewMatchables);
> > >>
> > >> // Process token alias definitions and set up the associated
> superclass
> > >> // information.
> > >> @@ -1679,9 +1671,8 @@ static unsigned getConverterOperandID(co
> > >> return ID;
> > >> }
> > >>
> > >> -
> > >> static void emitConvertFuncs(CodeGenTarget &Target, StringRef
> ClassName,
> > >> -
> std::vector<std::unique_ptr<MatchableInfo>> &Infos,
> > >> + std::forward_list<MatchableInfo> &Infos,
> > >> raw_ostream &OS) {
> > >> SetVector<std::string> OperandConversionKinds;
> > >> SetVector<std::string> InstructionConversionKinds;
> > >> @@ -1748,10 +1739,10 @@ static void emitConvertFuncs(CodeGenTarg
> > >> for (auto &II : Infos) {
> > >> // Check if we have a custom match function.
> > >> std::string AsmMatchConverter =
> > >> -
> II->getResultInst()->TheDef->getValueAsString("AsmMatchConverter");
> > >> +
> II.getResultInst()->TheDef->getValueAsString("AsmMatchConverter");
> > >> if (!AsmMatchConverter.empty()) {
> > >> std::string Signature = "ConvertCustom_" + AsmMatchConverter;
> > >> - II->ConversionFnKind = Signature;
> > >> + II.ConversionFnKind = Signature;
> > >>
> > >> // Check if we have already generated this signature.
> > >> if (!InstructionConversionKinds.insert(Signature))
> > >> @@ -1783,17 +1774,17 @@ static void emitConvertFuncs(CodeGenTarg
> > >> std::vector<uint8_t> ConversionRow;
> > >>
> > >> // Compute the convert enum and the case body.
> > >> - MaxRowLength = std::max(MaxRowLength, II->ResOperands.size()*2 +
> 1 );
> > >> + MaxRowLength = std::max(MaxRowLength, II.ResOperands.size() * 2
> + 1);
> > >>
> > >> - for (unsigned i = 0, e = II->ResOperands.size(); i != e; ++i) {
> > >> - const MatchableInfo::ResOperand &OpInfo = II->ResOperands[i];
> > >> + for (unsigned i = 0, e = II.ResOperands.size(); i != e; ++i) {
> > >> + const MatchableInfo::ResOperand &OpInfo = II.ResOperands[i];
> > >>
> > >> // Generate code to populate each result operand.
> > >> switch (OpInfo.Kind) {
> > >> case MatchableInfo::ResOperand::RenderAsmOperand: {
> > >> // This comes from something we parsed.
> > >> const MatchableInfo::AsmOperand &Op =
> > >> - II->AsmOperands[OpInfo.AsmOperandNum];
> > >> + II.AsmOperands[OpInfo.AsmOperandNum];
> > >>
> > >> // Registers are always converted the same, don't duplicate the
> > >> // conversion function based on them.
> > >> @@ -1916,7 +1907,7 @@ static void emitConvertFuncs(CodeGenTarg
> > >> if (Signature == "Convert")
> > >> Signature += "_NoOperands";
> > >>
> > >> - II->ConversionFnKind = Signature;
> > >> + II.ConversionFnKind = Signature;
> > >>
> > >> // Save the signature. If we already have it, don't add a new row
> > >> // to the table.
> > >> @@ -2602,23 +2593,21 @@ void AsmMatcherEmitter::run(raw_ostream
> > >> // Sort the instruction table using the partial order on classes. We
> use
> > >> // stable_sort to ensure that ambiguous instructions are still
> > >> // deterministically ordered.
> > >> - std::stable_sort(Info.Matchables.begin(), Info.Matchables.end(),
> > >> - [](const std::unique_ptr<MatchableInfo> &a,
> > >> - const std::unique_ptr<MatchableInfo> &b){
> > >> - return *a < *b;});
> > >> + Info.Matchables.sort();
> > >>
> > >> DEBUG_WITH_TYPE("instruction_info", {
> > >> for (const auto &MI : Info.Matchables)
> > >> - MI->dump();
> > >> + MI.dump();
> > >> });
> > >>
> > >> // Check for ambiguous matchables.
> > >> DEBUG_WITH_TYPE("ambiguous_instrs", {
> > >> unsigned NumAmbiguous = 0;
> > >> - for (unsigned i = 0, e = Info.Matchables.size(); i != e; ++i) {
> > >> - for (unsigned j = i + 1; j != e; ++j) {
> > >> - const MatchableInfo &A = *Info.Matchables[i];
> > >> - const MatchableInfo &B = *Info.Matchables[j];
> > >> + for (auto I = Info.Matchables.begin(), E =
> Info.Matchables.end(); I != E;
> > >> + ++I) {
> > >> + for (auto J = std::next(I); J != E; ++J) {
> > >> + const MatchableInfo &A = *I;
> > >> + const MatchableInfo &B = *J;
> > >>
> > >> if (A.couldMatchAmbiguouslyWith(B)) {
> > >> errs() << "warning: ambiguous matchables:\n";
> > >> @@ -2739,11 +2728,11 @@ void AsmMatcherEmitter::run(raw_ostream
> > >> unsigned MaxMnemonicIndex = 0;
> > >> bool HasDeprecation = false;
> > >> for (const auto &MI : Info.Matchables) {
> > >> - MaxNumOperands = std::max(MaxNumOperands,
> MI->AsmOperands.size());
> > >> - HasDeprecation |= MI->HasDeprecation;
> > >> + MaxNumOperands = std::max(MaxNumOperands, MI.AsmOperands.size());
> > >> + HasDeprecation |= MI.HasDeprecation;
> > >>
> > >> // Store a pascal-style length byte in the mnemonic.
> > >> - std::string LenMnemonic = char(MI->Mnemonic.size()) +
> MI->Mnemonic.str();
> > >> + std::string LenMnemonic = char(MI.Mnemonic.size()) +
> MI.Mnemonic.str();
> > >> MaxMnemonicIndex = std::max(MaxMnemonicIndex,
> > >> StringTable.GetOrAddStringOffset(LenMnemonic,
> false));
> > >> }
> > >> @@ -2767,8 +2756,9 @@ void AsmMatcherEmitter::run(raw_ostream
> > >> OS << " " << getMinimalTypeForRange(MaxMnemonicIndex)
> > >> << " Mnemonic;\n";
> > >> OS << " uint16_t Opcode;\n";
> > >> - OS << " " << getMinimalTypeForRange(Info.Matchables.size())
> > >> - << " ConvertFn;\n";
> > >> + OS << " " <<
> getMinimalTypeForRange(std::distance(Info.Matchables.begin(),
> > >> +
> Info.Matchables.end()))
> > >> + << " ConvertFn;\n";
> > >> OS << " " << getMinimalRequiredFeaturesType(Info)
> > >> << " RequiredFeatures;\n";
> > >> OS << " " << getMinimalTypeForRange(
> > >> @@ -2803,29 +2793,28 @@ void AsmMatcherEmitter::run(raw_ostream
> > >> OS << "static const MatchEntry MatchTable" << VC << "[] = {\n";
> > >>
> > >> for (const auto &MI : Info.Matchables) {
> > >> - if (MI->AsmVariantID != AsmVariantNo)
> > >> + if (MI.AsmVariantID != AsmVariantNo)
> > >> continue;
> > >>
> > >> // Store a pascal-style length byte in the mnemonic.
> > >> - std::string LenMnemonic = char(MI->Mnemonic.size()) +
> MI->Mnemonic.str();
> > >> + std::string LenMnemonic = char(MI.Mnemonic.size()) +
> MI.Mnemonic.str();
> > >> OS << " { " << StringTable.GetOrAddStringOffset(LenMnemonic,
> false)
> > >> - << " /* " << MI->Mnemonic << " */, "
> > >> - << Target.getName() << "::"
> > >> - << MI->getResultInst()->TheDef->getName() << ", "
> > >> - << MI->ConversionFnKind << ", ";
> > >> + << " /* " << MI.Mnemonic << " */, " << Target.getName()
> > >> + << "::" << MI.getResultInst()->TheDef->getName() << ", "
> > >> + << MI.ConversionFnKind << ", ";
> > >>
> > >> // Write the required features mask.
> > >> - if (!MI->RequiredFeatures.empty()) {
> > >> - for (unsigned i = 0, e = MI->RequiredFeatures.size(); i !=
> e; ++i) {
> > >> + if (!MI.RequiredFeatures.empty()) {
> > >> + for (unsigned i = 0, e = MI.RequiredFeatures.size(); i != e;
> ++i) {
> > >> if (i) OS << "|";
> > >> - OS << MI->RequiredFeatures[i]->getEnumName();
> > >> + OS << MI.RequiredFeatures[i]->getEnumName();
> > >> }
> > >> } else
> > >> OS << "0";
> > >>
> > >> OS << ", { ";
> > >> - for (unsigned i = 0, e = MI->AsmOperands.size(); i != e; ++i) {
> > >> - const MatchableInfo::AsmOperand &Op = MI->AsmOperands[i];
> > >> + for (unsigned i = 0, e = MI.AsmOperands.size(); i != e; ++i) {
> > >> + const MatchableInfo::AsmOperand &Op = MI.AsmOperands[i];
> > >>
> > >> if (i) OS << ", ";
> > >> OS << Op.Class->Name;
> > >>
> > >>
> > >> _______________________________________________
> > >> llvm-commits mailing list
> > >> llvm-commits at cs.uiuc.edu
> > >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141219/fb432386/attachment.html>
More information about the llvm-commits
mailing list