dexonsmith (Duncan P. N. Exon Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 18 2014, 10:33 AM (235 w, 5 d)

Recent Activity

Fri, Sep 21

dexonsmith added inline comments to D52322: Pass code-model through Module IR to LTO which will use is.
Fri, Sep 21, 8:34 AM

Thu, Sep 20

dexonsmith added inline comments to D52322: Pass code-model through Module IR to LTO which will use is.
Thu, Sep 20, 3:32 PM
dexonsmith added a comment to D52312: [DenseMapInfo] Add implementation for SmallVector of pointers..

I wonder, could these be replaced by TinyPtrVector? They're only 8B.

Interesting, I'll have a look. My only worry with this is that in the LSR and SLP use cases, it is very likely that the vector contains more than 1 element.

Thu, Sep 20, 3:29 PM
dexonsmith added a comment to D52312: [DenseMapInfo] Add implementation for SmallVector of pointers..

I'm concerned about propagating this pattern. DenseSet, like DenseMap, is tuned for small keys and values. SmallVector<void*,4> is not exactly small, at 48B. And this patch makes it easy to throw much larger SmallVectors into these containers.

Thu, Sep 20, 9:50 AM

Mon, Sep 17

dexonsmith accepted D30882: Add a callback for __has_include and use it for dependency scanning.

LGTM too.

Mon, Sep 17, 3:03 PM
dexonsmith accepted D51540: [ThinLTO] Update LangRef doc for summary parsing.

LGTM.

Mon, Sep 17, 2:49 PM

Fri, Sep 7

dexonsmith updated subscribers of D51789: [clang] Add the exclude_from_explicit_instantiation attribute.

+cfe-commits
-llvm-commits

Fri, Sep 7, 8:46 AM

Wed, Sep 5

dexonsmith added a reviewer for D51664: [IR] Lazily number instructions for local dominance queries: ahatanak.

This is awesome to see!

Wed, Sep 5, 5:43 PM
dexonsmith added a comment to D51440: [ToolChains] Link to compiler-rt with -L + -l when possible.

I do prefer the current approach especially on Darwin. Some concerns of switching to use "-L + -l" are:

  1. clang and compiler-rt are rev-locked. Inferring the compiler-rt from resource-dir and passing to linker with the full path can prevent mistakes of mixing them up.
  2. This change does change linker semantics on Darwin. ld64 treats the inputs from command line with highest priority. Currently ld64 will use compiler-rt symbols before any other libraries passed in with -l flag. If clang decide to pass compiler-rt with -l flag, any other libraries (static or dynamic) that passed with -l flag will takes the priority over compiler-rt. This can lead to unexpected behavior.
Wed, Sep 5, 4:37 PM

Fri, Aug 31

dexonsmith added inline comments to D51540: [ThinLTO] Update LangRef doc for summary parsing.
Fri, Aug 31, 7:08 AM
dexonsmith added a comment to D51497: [WIP][ORC][ThinLTO] Early ThinLTO-JIT prototype and basic tests for next-gen ORC APIs.

You should check in LLVM textual IR files (.ll) instead of C++ source files or object files. Using clang -S -emit-llvm (or llvm-dis some-thinlto-bitcode-file.o) should get you started.

Yes that would be great, but wouldn't it require IR representation for summaries? I didn't double-check myself, but there's a note in the language ref, "that temporarily the summary entries are skipped when parsing the assembly". Not sure if I understand that correctly, but I was under the impression that we can't have summaries in IR files yet.
https://llvm.org/docs/LangRef.html#thinlto-summary

Fri, Aug 31, 6:33 AM
dexonsmith added inline comments to D51507: Allow all supportable attributes to be used with #pragma clang attribute..
Fri, Aug 31, 6:23 AM

Thu, Aug 30

dexonsmith added inline comments to D51507: Allow all supportable attributes to be used with #pragma clang attribute..
Thu, Aug 30, 2:52 PM
dexonsmith updated subscribers of D51497: [WIP][ORC][ThinLTO] Early ThinLTO-JIT prototype and basic tests for next-gen ORC APIs.

Not sure what's the best way to get rid of the binary object files. IIUC the ThinLTO tests are lit-based, so they build their summary-enhanced objects with clang and test via llvm-lto2 tool. Maybe I can turn all the unit tests into lit-test and add a hook to llvm-lto2? Don't know if there is a way to emit object files with summaries using yaml2obj or so? Any ideas?

Thu, Aug 30, 12:40 PM

Wed, Aug 29

dexonsmith updated subscribers of D30009: Add support for '#pragma clang attribute'.

One consequence of this patch (see https://reviews.llvm.org/rL341002) is that adding documentation to an existing attribute changes the behavior of Clang. That does not seem acceptable to me: documentation improvements should not change which attributes we allow this pragma to appertain to.

If we really want an "only documented attribtues can use this feature" rule (and I'm really not convinced that is a useful rule to enforce

Wed, Aug 29, 6:09 PM

Mon, Aug 27

dexonsmith added a comment to D51240: Add a flag to remap manglings when reading profile data information..

Re "Why not":

The common use model of instrumentation based PGO and SamplePGO are quite different. The former usually uses 'fresh' profile that matches the source. If there are massive code refactoring or ABI changes, the user can simply regenerate the profile. For the latter, the profile data is usually collected from production binaries, so the source is almost always newer than the profile.

Mon, Aug 27, 10:19 AM

Aug 24 2018

dexonsmith added a reviewer for D51240: Add a flag to remap manglings when reading profile data information.: vsk.

In particular, I don't see much need to do this for instrumentation based PGO [...]

Aug 24 2018, 6:00 PM

Aug 23 2018

dexonsmith added a reviewer for D51170: [libc++] Remove race condition in std::async: kubamracek.
Aug 23 2018, 1:18 PM

Aug 15 2018

dexonsmith accepted D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI.

LGTM if thakis is happy.

Aug 15 2018, 2:39 PM
dexonsmith added inline comments to D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI.
Aug 15 2018, 9:25 AM

Aug 14 2018

dexonsmith accepted D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES.

I have pissed in this area, too - See https://bugs.llvm.org/show_bug.cgi?id=38495 and the proposed resolution here: https://bugs.llvm.org/attachment.cgi?id=20692
How about I just make this change as part of that fix?

Aug 14 2018, 11:02 AM
dexonsmith added a comment to D50599: [VERY-WIP] [ItaniumDemangle] Add ability to re-serialize the parsed AST .

I have one process question though. What's the current workflow for merging changes between the demangler copies in libc++abi and llvm? Right now they're mostly identical except for a few macros, but CRTP will require putting a lot of stuff into the header (and into a namespace), so they will diverge more. Do you think it would make sense to factor the common bits into a separate (.inc ?) file which can be kept identical? Then cxa_demangle.cpp could just #include that file into an anonymous namespace and llvm could include that into the llvm namespace (from a header file).

I have an ugly perl script that I use to patch changes from libcxxabi to llvm (see the end of this message). The basic idea for compatibility in libcxxabi is ItaniumDemangle.cpp maps to libcxxabi/src/cxa_demangle, and the other headers in llvm/lib/Demangle/* map to their equivalents in libcxxabi/src/demangle/*. I would like to keep them identical to keep patching easy, so any changes to the file structure in llvm should also be done in libcxxabi.
I think that we should move the Node hierarchy into it's own file ItaniumNode.h, and the CRTP parser into one, ItaniumParser.h which would include ItaniumNode.h, and put all the declarations in the namespace llvm::itanium_demangler (or an anon namespace in libcxxabi). That would allow us to implement ItaniumPartialDemangler in a separate file (which wouldn't map to anything in libcxxabi), which would be nice. ItaniumDemangle.cpp could just be a #include "ItaniumParser.h" and the implementation of itaniumDemangle(), or __cxa_demangle in libcxxabi. Does that sounds reasonable? CCing @rsmith, who expressed some interest in moving the AST/parser to a header, and @dexonsmith, who's had some thoughts on this before.

Aug 14 2018, 11:01 AM

Jul 27 2018

dexonsmith added a comment to D49914: [libc++] Add the _LIBCPP_HIDE_FROM_ABI_AFTER_V1 macro.

This looks correct to me, but I wouldn't mind having someone else take a look too.

Jul 27 2018, 9:27 AM

Jul 26 2018

dexonsmith updated subscribers of D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.

Hi Peter,

Jul 26 2018, 6:02 PM
dexonsmith created D49881: docs: Emphasize ArrayRef over SmallVectorImpl.
Jul 26 2018, 3:29 PM
dexonsmith accepted D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY.

LGTM. Assuming this doesn't break the ABI list checks.

I talked to @ldionne about if we wanted to land this right before 7.0, but I think we can. If we have to disable the internal linkage change, we can at least keep the new naming scheme and start transitioning to it.

I'd like to get input from @mclow.lists and @dexonsmith about whether we should move forward with this before we branch 7.0, or wait after the release. This is a wide reaching change so I want to share responsibility ;-)

Jul 26 2018, 3:02 PM
dexonsmith committed rL338071: ADT: Document advantages of SmallVector<T,0> over std::vector.
ADT: Document advantages of SmallVector<T,0> over std::vector
Jul 26 2018, 2:30 PM
dexonsmith closed D49748: ADT: Document advantages of SmallVector<T,0> over std::vector.

Committed in r338071.

Jul 26 2018, 2:30 PM

Jul 25 2018

dexonsmith accepted D49808: [libc++] Factor duplicate code into function templates.

Nice cleanup! LGTM.

Jul 25 2018, 11:46 AM

Jul 24 2018

dexonsmith created D49748: ADT: Document advantages of SmallVector<T,0> over std::vector.
Jul 24 2018, 12:09 PM
dexonsmith committed rL337820: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms.
ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms
Jul 24 2018, 4:34 AM
dexonsmith closed D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms.

Committed in r337820.

Jul 24 2018, 4:34 AM

Jul 23 2018

dexonsmith added a comment to D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY.

FWIW this LGTM, but since Marshall already commented you'll need his sign-off as well.

Jul 23 2018, 9:45 AM
dexonsmith accepted D49278: [ThinLTO] Ensure the TargetLibraryInfo is constructed early enough.

LGTM.

Jul 23 2018, 9:43 AM

Jul 20 2018

dexonsmith accepted D49340: Fix IR Printing test.

LGTM too.

Jul 20 2018, 8:33 AM

Jul 19 2018

dexonsmith added a comment to D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.

Hmm... that bot was green again in r337508, before I had a chance to revert. Maybe the blame-list was faulty.

Jul 19 2018, 5:50 PM
dexonsmith committed rL337514: Reapply "ADT: Shrink size of SmallVector by 8B on 64-bit platforms".
Reapply "ADT: Shrink size of SmallVector by 8B on 64-bit platforms"
Jul 19 2018, 5:50 PM
dexonsmith added inline comments to D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms.
Jul 19 2018, 5:35 PM
dexonsmith added a comment to D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.
Jul 19 2018, 5:20 PM
dexonsmith reopened D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.

Reverted in r337511 due to a bot failure:

#8 0x000055581f2895d8 (/b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/bin/clang-7+0x1eb45d8)
#9 0x000055581f294323 llvm::ConstantAggrKeyType<llvm::ConstantArray>::create(llvm::ArrayType*) const /b/sanitizer-x86_64-linux-autoconf/build/llvm/lib/IR/ConstantsContext.h:409:0
#10 0x000055581f294323 llvm::ConstantUniqueMap<llvm::ConstantArray>::create(llvm::ArrayType*, llvm::ConstantAggrKeyType<llvm::ConstantArray>, std::pair<unsigned int, std::pair<llvm::ArrayType*, llvm::ConstantAggrKeyType<llvm::ConstantArray> > >&) /b/sanitizer-x86_64-linux-autoconf/build/llvm/lib/IR/ConstantsContext.h:635:0
#11 0x000055581f294323 llvm::ConstantUniqueMap<llvm::ConstantArray>::getOrCreate(llvm::ArrayType*, llvm::ConstantAggrKeyType<llvm::ConstantArray>) /b/sanitizer-x86_64-linux-autoconf/build/llvm/lib/IR/ConstantsContext.h:654:0
#12 0x000055581f2944cb llvm::ConstantArray::get(llvm::ArrayType*, llvm::ArrayRef<llvm::Constant*>) /b/sanitizer-x86_64-linux-autoconf/build/llvm/lib/IR/Constants.cpp:964:0
#13 0x000055581fa27e19 llvm::SmallVectorBase::size() const /b/sanitizer-x86_64-linux-autoconf/build/llvm/include/llvm/ADT/SmallVector.h:53:0
#14 0x000055581fa27e19 llvm::SmallVectorImpl<llvm::Constant*>::resize(unsigned long) /b/sanitizer-x86_64-linux-autoconf/build/llvm/include/llvm/ADT/SmallVector.h:347:0
#15 0x000055581fa27e19 (anonymous namespace)::EmitArrayConstant(clang::CodeGen::CodeGenModule&, clang::ConstantArrayType const*, llvm::Type*, unsigned int, llvm::SmallVectorImpl<llvm::Constant*>&, llvm::Constant*) /b/sanitizer-x86_64-linux-autoconf/build/llvm/tools/clang/lib/CodeGen/CGExprConstant.cpp:669:0

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/26526

Jul 19 2018, 5:16 PM
dexonsmith committed rL337511: Revert "ADT: Shrink size of SmallVector by 8B on 64-bit platforms".
Revert "ADT: Shrink size of SmallVector by 8B on 64-bit platforms"
Jul 19 2018, 5:15 PM
dexonsmith added a comment to D49532: [DebugInfoMetadata] Added endianity field in DIBasicType to hold DW_AT_endianity attribute for DW_TAG_basic_type..

Probably squirrel it away in the generic 'flags' bucket?

Seems reasonable to me. Might need a tristate or something, but that's what enums and masking are for.

Jul 19 2018, 4:40 PM
dexonsmith closed D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.

Committed in r337504.

Jul 19 2018, 3:38 PM
dexonsmith added inline comments to D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms.
Jul 19 2018, 3:37 PM
dexonsmith committed rL337504: ADT: Shrink size of SmallVector by 8B on 64-bit platforms.
ADT: Shrink size of SmallVector by 8B on 64-bit platforms
Jul 19 2018, 3:35 PM

Jul 17 2018

dexonsmith accepted D49458: Support implicit _Atomic struct load / store.
Jul 17 2018, 8:28 PM

Jul 16 2018

dexonsmith added a comment to D49340: Fix IR Printing test.

Shouldn't this have -print-module in the command-line, since that's what this is supposed to be testing? I must have missed that in the original review.

Jul 16 2018, 9:47 AM

Jul 11 2018

dexonsmith closed D48723: Fix IRPrinting bug.

Committed in r336869!

Jul 11 2018, 4:56 PM
dexonsmith committed rL336869: IR: Skip -print-*-all after -print-*.
IR: Skip -print-*-all after -print-*
Jul 11 2018, 4:35 PM

Jul 10 2018

dexonsmith added a dependency for D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms: D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.
Jul 10 2018, 5:15 PM
dexonsmith added a dependent revision for D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms: D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms.
Jul 10 2018, 5:15 PM
dexonsmith created D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms.
Jul 10 2018, 5:14 PM
dexonsmith added a comment to D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.

Ping!

Jul 10 2018, 4:24 PM
dexonsmith added a comment to D48991: Docs: Spell C++ lambdas like functions, not variables.

Only lambdas, and not other functors (like std::function, etc)?

So we could end up with code like:

MyFunctor = myLambda;

Though I suppose that's OK - and it's only in the local scope of a function
& not like any interesting object-like operations (such as assignment) can
be done with the lambda as the subject anyway, so it does look a lot like a
function... (except you can't assign it to a function pointer - but at
least the lambda should be nearby in the code so that's not too confusing)

Jul 10 2018, 4:23 PM
dexonsmith added a comment to D49138: [LTO] Handle __imp_ (dllimport) symbols consistently with lld.

Thanks! (I'm not comfortable signing off though, since I don't know much about dllimport.)

Jul 10 2018, 9:36 AM
dexonsmith added a comment to D49138: [LTO] Handle __imp_ (dllimport) symbols consistently with lld.

Is it impossible to also test this locally (in the LLVM repo)? It will be hard to hack on this file if there aren't local tests.

Jul 10 2018, 9:03 AM
dexonsmith accepted D49090: [ThinLTO] Escape module paths when printing.

LGTM too.

Jul 10 2018, 9:00 AM

Jul 9 2018

dexonsmith added inline comments to D46274: [Support] Harden JSON against invalid UTF-8..
Jul 9 2018, 11:51 AM
dexonsmith requested changes to D49090: [ThinLTO] Escape module paths when printing.
Jul 9 2018, 11:25 AM
dexonsmith accepted D49051: [libclang] check that the cursor is declaration before trying to evaluate the cursor like a declaration.

LGTM.

Jul 9 2018, 8:25 AM

Jul 7 2018

dexonsmith added a comment to D49030: Add OrderedSet, with constant-time insertion and removal, and random access iteration..

We already have SetVector which is widely used for these patterns. If we need both, we need a clear explanation of the difference and why we need both (IE, why users of one can't use the other).

The remove operation on a SetVector can cost linear time and this might be an issue in some cases. Please have a look at the review comments of https://reviews.llvm.org/D48372

Jul 7 2018, 7:56 AM

Jul 5 2018

dexonsmith added a comment to D48857: Also search BitcodeFiles for exclude-lib symbols.

It may make sense to get a consensus and update the coding standard
document, as it is not entirely clear. If that's the case, I'd vote for
CamelCase. :)

Jul 5 2018, 1:00 PM
dexonsmith added a comment to D48991: Docs: Spell C++ lambdas like functions, not variables.

I've started an RFC on llvm-dev to discuss whether we should make this change:
http://lists.llvm.org/pipermail/llvm-dev/2018-July/124466.html

Jul 5 2018, 12:58 PM
dexonsmith created D48991: Docs: Spell C++ lambdas like functions, not variables.
Jul 5 2018, 12:54 PM
dexonsmith added a comment to D48857: Also search BitcodeFiles for exclude-lib symbols.

Is that part of our coding standard? In lld, all local variables holding
function-like objects are in CamelCase, not in camelCase.

Jul 5 2018, 12:46 PM
dexonsmith accepted D48723: Fix IRPrinting bug.

Anyway, if this is not the right way to do, then we should change BitcodeWriter to a non-analysis pass I guess? There should be no difference between these 2 passes on whether it is an analysis or not IMHO

Jul 5 2018, 12:13 PM
dexonsmith added inline comments to D48857: Also search BitcodeFiles for exclude-lib symbols.
Jul 5 2018, 12:04 PM
dexonsmith added a comment to D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY.

The lldb bot started failing very recently and the blamelist hints at this change.

http://green.lab.llvm.org/green/job/lldb-cmake/7777/

Can you please take a look?

For your convenience, this is failing building LibCxx testcases with a linker error:

Build Command Output:
Undefined symbols for architecture x86_64:
  "std::__1::__vector_base_common<true>::__vector_base_common()", referenced from:
      std::__1::__vector_base<int, std::__1::allocator<int> >::__vector_base() in main.o
      std::__1::__vector_base<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >::__vector_base() in main.o
ld: symbol(s) not found for architecture x86_64
clang-7: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [a.out] Error 1
Jul 5 2018, 11:37 AM
dexonsmith added a comment to D48983: [libc++] Replace incorrect visibility on a streambuf method.

It's pretty obvious that this is the problem when looking at https://reviews.llvm.org/D48892 (grep for __pbump), but TBH I haven't been able to run the check-cxx-abilist test locally. Mine fails with like 500 differences, so there's got to be something I'm doing wrong.

Jul 5 2018, 11:30 AM
dexonsmith accepted D48983: [libc++] Replace incorrect visibility on a streambuf method.

LGTM.

Jul 5 2018, 11:29 AM
dexonsmith accepted D48939: CallGraph passes: iterate over all functions rather than just externally visible ones.

LGTM.

Jul 5 2018, 11:26 AM

Jul 4 2018

dexonsmith added a reviewer for D48939: CallGraph passes: iterate over all functions rather than just externally visible ones: dexonsmith.

Awesome. I wonder, would it be worth measuring compile-time impact for optimized builds?

Jul 4 2018, 10:04 AM

Jul 3 2018

dexonsmith added inline comments to D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY.
Jul 3 2018, 12:57 PM
dexonsmith added a reviewer for D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY: mclow.lists.

FWIW, this change LGTM... but I wouldn't mind hearing from Eric or Marshall before commit.

Jul 3 2018, 12:50 PM

Jul 2 2018

dexonsmith added a comment to D48698: [ThinLTO] Fix printing of module paths for distributed backend indexes.

What was the numbering before?

Jul 2 2018, 10:50 AM

Jun 29 2018

dexonsmith added a comment to D48723: Fix IRPrinting bug.

No don't get me wrong, this patch is supposed to fix the issue in the LegacyPassManager, and I do not intend to change anything about the new pass manager in this patch.

As to implementing print-after-all in the new pass manager, I do not know what should be the best way to do yet... Your idea seems to be a good solution.

Jun 29 2018, 10:18 AM

Jun 28 2018

dexonsmith resigned from D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple.
Jun 28 2018, 8:56 PM
dexonsmith added a comment to D48723: Fix IRPrinting bug.

I'm not sure this technique will work in the new pass manager, since analyses are run on-demand IIRC. I wonder if it would be better to leave these as passes, and implement -print-after-all by inserting -print-module/etc. passes into the pipeline as the pipeline is being constructed. You could avoid adding extra -print-module statements in cases where that pass already exists.

Jun 28 2018, 5:51 PM
dexonsmith requested changes to D48723: Fix IRPrinting bug.

This seems like a reasonable thing to do. Please add tests to check the output of both pass managers.

Jun 28 2018, 9:02 AM
dexonsmith added inline comments to D48693: [WebAssembly] LTO: Fix signatures of bitcode symbols.
Jun 28 2018, 8:47 AM

Jun 27 2018

dexonsmith added a reviewer for D48694: [libc++abi] Limit libc++ header search to specified paths: ldionne.

What's the effective change on the command-line? Does this map to -nostdinc? To -nostdinc++?

Jun 27 2018, 5:48 PM
dexonsmith updated subscribers of D48675: [libc++abi] Limit libc++ header search to specified paths.

Please close this and open a new one. Adding cfe-commits after the fact will fail to send the patch/description to cfe-commits, and not everyone is on llvm-commits.

Jun 27 2018, 5:40 PM
dexonsmith resigned from D48369: [CodeGen] Make block removal order deterministic in CodeGenPrepare.

Resigning as reviewer (thanks for fixing the complexity); I'll let the other reviewers sort out if SmallVector is an even better solution here.

Jun 27 2018, 8:52 AM

Jun 26 2018

dexonsmith added inline comments to D48241: [DebugInfo] Emit ObjC methods as part of interface..
Jun 26 2018, 9:44 AM
dexonsmith accepted D47906: [ThinLTO] Add testing of summary index parsing to a couple CFI tests.

LGTM.

Jun 26 2018, 8:29 AM

Jun 25 2018

dexonsmith added a comment to D47842: [ThinLTO] Add string saver onto index for value names.
In D47842#1142966, @pcc wrote:
In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

My recollection is that BumpPtrAllocator does its first heap allocation on construction. The point was to avoid that allocation when it was unnecessary. Have I remembered incorrectly?

It doesn't appear to do that. BumpPtrAllocator's default constructor (http://llvm-cs.pcc.me.uk/include/llvm/Support/Allocator.h#152) uses = default;.

Jun 25 2018, 3:52 PM
dexonsmith accepted D47844: [ThinLTO] Compute GUID directly from GV when building per-module index.

Okay, this LGTM then.

Jun 25 2018, 3:50 PM
dexonsmith added a comment to D47842: [ThinLTO] Add string saver onto index for value names.
In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

Jun 25 2018, 3:44 PM
dexonsmith added a comment to D47844: [ThinLTO] Compute GUID directly from GV when building per-module index.

What's the potential overhead of this choice (memory, runtime)?

Jun 25 2018, 1:58 PM
dexonsmith accepted D47842: [ThinLTO] Add string saver onto index for value names.

LGTM.

Jun 25 2018, 1:54 PM

Jun 23 2018

dexonsmith updated the diff for D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.

Updating the patch since I decided to just lift set_size() up to SmallVectorBase. My ASan-ified check-llvm and check-clang came back happy.

Jun 23 2018, 4:11 PM
dexonsmith closed D48516: ADT: Use EBO to shrink SmallVector size 1.

Committed in r335421.

Jun 23 2018, 11:49 AM
dexonsmith committed rL335421: ADT: Use EBO to shrink SmallVector size 1.
ADT: Use EBO to shrink SmallVector size 1
Jun 23 2018, 11:44 AM
dexonsmith added a dependency for D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms: D48516: ADT: Use EBO to shrink SmallVector size 1.
Jun 23 2018, 8:40 AM
dexonsmith added a dependent revision for D48516: ADT: Use EBO to shrink SmallVector size 1: D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.
Jun 23 2018, 8:40 AM
dexonsmith created D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.
Jun 23 2018, 8:40 AM

Jun 22 2018

dexonsmith created D48516: ADT: Use EBO to shrink SmallVector size 1.
Jun 22 2018, 8:54 PM
dexonsmith accepted D47905: [ThinLTO] Parse module summary index from assembly.

I still think you might be able to use an Optional (see my inline comment) but I don't need to see this again.

Jun 22 2018, 7:46 PM

Jun 21 2018

dexonsmith requested changes to D48369: [CodeGen] Make block removal order deterministic in CodeGenPrepare.
Jun 21 2018, 8:41 AM

Jun 20 2018

dexonsmith requested changes to D47905: [ThinLTO] Parse module summary index from assembly.

I just have a couple of additional (minor) comments on the code, although I admit it was hard to be thorough as a it's a monolithic patch. I might have preferred if this were committed incrementally, adding support for 1-2 constructs at a time. Because the lexing/parsing code is fairly mechanical, I think splitting it up now would be quite likely to introduce errors so I'm not going to ask for that.

Jun 20 2018, 9:38 AM