Page MenuHomePhabricator

Inline basic_string::erase for fastpath where __n == npos
ClosedPublic

Authored by mvels on Jan 30 2020, 1:43 PM.

Details

Summary

This change checks for the case where people want to erase a string to the end, i.e., __n == npos, and inlines the call if so.

This also demonstrates keeping the ABI intact for V1, but inlining the erase() method for unstable.

Diff Detail

Event Timeline

mvels created this revision.Jan 30 2020, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2020, 1:43 PM
mvels updated this revision to Diff 241803.Jan 31 2020, 2:03 PM

Make inlining subject to both unstable ABI and V2

mvels updated this revision to Diff 241811.Jan 31 2020, 2:24 PM

Added clarifying comments

ldionne requested changes to this revision.Feb 19 2020, 12:35 PM

You'll need to update the ABI lists for the unstable ABI to account for the addition of __erase_external. Try running ninja check-cxx-abilist.

Lurkers: Martijn and I spoke at length and we decided to try and have two fully separate lists of exported symbols for the stable and unstable ABIs. We think this will make it clearer what's exported in what configuration, etc. In particular, I didn't like the fact that in the stable ABI this patch started exporting __erase_external even though it would never actually be used from the dylib (cause otherwise it's an ABI break for back-deployment). Martijn nicely agreed to prototype a patch that splits the two lists out, and we'll see which way we go based on that patch.

libcxx/include/string
3004

'externally instantiated' ?

3039

In what way is this unstable?

This revision now requires changes to proceed.Feb 19 2020, 12:35 PM
mvels added a comment.Feb 19 2020, 2:17 PM

The external template list changes (split into explicit V1 and UNSTABLE versions) are in https://reviews.llvm.org/D74870

I'll rebase this change on that one (assuming arc phabricator figures out me doing so)

mvels updated this revision to Diff 245533.Feb 19 2020, 2:50 PM
mvels marked 3 inline comments as done.
  • Fix external template list
mvels added inline comments.Feb 19 2020, 2:52 PM
libcxx/include/string
3039

I agree this comment is confusing, as it details 'here is ABI detail' which we deliberately keep as a separate concern in __string.

So I removed the comment, if you have suggestions for a comment that may be helpful for a future reader or editor that may not be aware of this function potentially available for inlining under unstable, I welcome alternatives.

mvels added a comment.Feb 19 2020, 2:54 PM

Is there a way I can easily tell phabricator that this is baselined against https://reviews.llvm.org/D74870 ?

I admit I don't understand phabricator well, or the mysterious ways it works.

On my local repo I rebased my branch, which works like a charm as that is ..... basic git.
Unfortunately arc diff then updates the wrong review, so I had to fix up both this one and D74870,

I also figured setting 'parent revision' would establish this (not so much).

Should I RTFM? :)

mvels updated this revision to Diff 245534.Feb 19 2020, 2:57 PM
  • Fix external template list + typo

Is there a way I can easily tell phabricator that this is baselined against https://reviews.llvm.org/D74870 ?

I admit I don't understand phabricator well, or the mysterious ways it works.

On my local repo I rebased my branch, which works like a charm as that is ..... basic git.
Unfortunately arc diff then updates the wrong review, so I had to fix up both this one and D74870,

I also figured setting 'parent revision' would establish this (not so much).

Should I RTFM? :)

Try arc diff HEAD^.

We should just adjust the .arcconfig for LLVM to do this automatically ... I'll put up a patch.

mvels updated this revision to Diff 245938.Feb 21 2020, 11:43 AM

Inline basic_string::erase for fastpath where __n == npos

Some renames, rediffed on existing ABI lists

mvels updated this revision to Diff 245943.Feb 21 2020, 11:52 AM
  • Cleaned up return types
  • Udpated ABI list
mvels updated this revision to Diff 245946.Feb 21 2020, 11:55 AM
  • Cosmetics: un-delete empty line

Is there a way I can easily tell phabricator that this is baselined against https://reviews.llvm.org/D74870 ?

I admit I don't understand phabricator well, or the mysterious ways it works.

On my local repo I rebased my branch, which works like a charm as that is ..... basic git.
Unfortunately arc diff then updates the wrong review, so I had to fix up both this one and D74870,

I also figured setting 'parent revision' would establish this (not so much).

Should I RTFM? :)

Try arc diff HEAD^.

We should just adjust the .arcconfig for LLVM to do this automatically ... I'll put up a patch.

D74990

EricWF accepted this revision.Feb 23 2020, 4:08 PM

LGTM.

ldionne accepted this revision.Feb 24 2020, 7:11 AM
This revision is now accepted and ready to land.Feb 24 2020, 7:11 AM
EricWF added inline comments.Feb 24 2020, 12:18 PM
libcxx/include/string
1579

Nit: I was fond of using external to denote that a function was designed as such.

3008

I don't think any operation in this function can throw. Should we mark it noexcept?

mvels updated this revision to Diff 246783.Feb 26 2020, 10:27 AM
mvels marked 2 inline comments as done.

Small twiddles from review

ldionne accepted this revision.Feb 26 2020, 10:36 AM
This revision was automatically updated to reflect the committed changes.

Hi, I believe this patch is causing the following undefined symbol error we're seeing in our 2 stage mac builders:

[367/3568] Linking CXX executable bin/llvm-tblgen
FAILED: bin/llvm-tblgen 
: && /b/s/w/ir/k/recipe_cleanup/clangINjhKi/llvm_build_dir/./bin/clang++  -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -flto -fdebug-prefix-map=/b/s/w/ir/k/recipe_cleanup/clangINjhKi/llvm_build_dir/tools/clang/stage2-bins=../recipe_cleanup/clangINjhKi/llvm_build_dir/tools/clang/stage2-bins -fdebug-prefix-map=/b/s/w/ir/k/llvm-project/= -no-canonical-prefixes -O3 -DNDEBUG -Wl,-search_paths_first -Wl,-headerpad_max_install_names -nostdlib++ /b/s/w/ir/k/cipd/lib/libc++.a -flto -Wl,-lto_library -Wl,/b/s/w/ir/k/recipe_cleanup/clangINjhKi/llvm_build_dir/./lib/libLTO.dylib    -Wl,-dead_strip utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmMatcherEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterInst.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/Attributes.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CallingConvEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeEmitterGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenDAGPatterns.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenHwModes.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenInstruction.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenMapTable.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenSchedule.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenTarget.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherOpt.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcher.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAPacketizerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DisassemblerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/ExegesisEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/FastISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/FixedLenDecoderEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/GICombinerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/GlobalISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InfoByHwMode.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrInfoEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrDocsEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/IntrinsicEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptParserEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptRSTEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/PredicateExpander.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/PseudoLoweringEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RISCVCompressInstEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterBankEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterInfoEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SDNodeProperties.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SearchableTableEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetFeatureInfo.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/TableGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/Types.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86DisassemblerTables.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86EVEX2VEXTablesEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86FoldTablesEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86ModRMFilters.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86RecognizableInstr.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/WebAssemblyDisassemblerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CTagsEmitter.cpp.o  -o bin/llvm-tblgen  -Wl,-rpath,@loader_path/../lib lib/libLLVMSupport.a lib/libLLVMTableGen.a lib/libLLVMTableGenGlobalISel.a lib/libLLVMSupport.a /usr/lib/libz.dylib -lm lib/libLLVMDemangle.a && :
Undefined symbols for architecture x86_64:
  "std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> >::__erase_external_with_move(unsigned long, unsigned long)", referenced from:
      (anonymous namespace)::AsmWriterEmitter::EmitPrintInstruction(llvm::raw_ostream&) in lto.o
      UnescapeString(std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> >&) in lto.o
ld: symbol(s) not found for architecture x86_64
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

Could you take a look and send out a fix or revert? Thanks.

Builder: https://luci-milo.appspot.com/p/fuchsia/builders/prod/clang-mac-x64/b8887357725525428704