Page MenuHomePhabricator

steven_wu (Steven Wu)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 4 2014, 3:46 PM (438 w, 2 d)

Recent Activity

Thu, Mar 16

steven_wu added inline comments to D142084: [RFC][X86][MemFold] Upgrade the mechanism of auto-generated Memory Folding Table.
Thu, Mar 16, 11:04 AM · Restricted Project, Restricted Project

Feb 28 2023

steven_wu resigned from D139816: [LTO] Don't generate invalid modules if "LTOPostLink" MD already exists.
Feb 28 2023, 9:33 AM · Restricted Project, Restricted Project

Feb 7 2023

steven_wu accepted D143428: [clang][deps] Fix race condition.
Feb 7 2023, 9:20 AM · Restricted Project, Restricted Project

Feb 4 2023

steven_wu added a comment to rGc294bdd64768: libLTO.{so,dylib}: remove unused non-LTO symbols.

I will let @davide to decide. I will talk to him offline if he didn't reply here. Looking at the libLTO shipped by Apple, it actually have more symbols than listed here so it could be reasonable just keep the minimal list of symbols intended here.

Feb 4 2023, 12:46 PM · Restricted Project, Restricted Project

Feb 3 2023

steven_wu added a comment to D143229: [AutoUpgrade] Add flag to disable autoupgrading debug info.

check llvm.ident instead

If my source module is bitcode upgraded, it won't get a new llvm.ident right? Then the upgrade pass will be wrongly skipped. I think we need a LLVM code generator version somewhere in the IR

it the bitcode was upgraded, then during the upgrade it should already have had UpgradeDebugInfo called on it (with the expected LLVM revision) right?

Feb 3 2023, 2:53 PM · Restricted Project, Restricted Project
steven_wu added a comment to D143229: [AutoUpgrade] Add flag to disable autoupgrading debug info.

check llvm.ident instead

Feb 3 2023, 12:56 PM · Restricted Project, Restricted Project
steven_wu added a comment to D143229: [AutoUpgrade] Add flag to disable autoupgrading debug info.

I don't think this restriction works for Apple. Compatibility is important for users that needs external dependencies that they do not control. If the debug info upgrader is expensive for you, we can probably add a fast check to skip that, or just give user a flag to control that behavior.

Feb 3 2023, 9:05 AM · Restricted Project, Restricted Project

Feb 2 2023

steven_wu committed rG0480748ea672: [DeclContext] Sort the Decls before adding into DeclContext (authored by steven_wu).
[DeclContext] Sort the Decls before adding into DeclContext
Feb 2 2023, 3:16 PM · Restricted Project, Restricted Project
steven_wu closed D141625: [DeclContext] Sort the Decls before adding into DeclContext.
Feb 2 2023, 3:16 PM · Restricted Project, Restricted Project
steven_wu updated subscribers of rGc294bdd64768: libLTO.{so,dylib}: remove unused non-LTO symbols.

@MaskRay Those disassembler symbols are actually used by otool, see: https://opensource.apple.com/source/cctools/cctools-973.0.1/otool/arm_disasm.c.auto.html

Feb 2 2023, 3:16 PM · Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

I don't think it is feasible with current tool to write a test that check semantically the order of decls in clang module. (Let me know if that was wrong). The current test already unfortunately relies on the module layout assuming op13 is the field for anonymous declaration number.

Sure enough - having these things have semantic identifiers rather than numbers would help.

Ah, perhaps I'm just confused - I'm not sure why a similar test, that tested a different order of the op13 fields wouldn't've shown a failure without reverse iteration and then failed with reverse iteration (or the other way around) - and then could be updated with a different ordering with the fix.

Sorry, I'm clearly not making much sense here. Yes, I think the test should be reduced further (while still showing the failure in either forward or reverse iteration - but, yes, losing coverage in the real world rehashing situation) it'd make the test shorter and more maintainable (to @akyrtzi's point about future changes that introduce benign reordering) but it's not the worst example of long tests to tickle hash instability. *shrug*

I'm not insisting on it/blocking this patch from going forward without addressing this issue. Carry on.

Feb 2 2023, 3:05 PM · Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

I don't think it is feasible with current tool to write a test that check semantically the order of decls in clang module. (Let me know if that was wrong). The current test already unfortunately relies on the module layout assuming op13 is the field for anonymous declaration number. Adding more dependency on the exact layout of the clang module will make the test really fragile to any clang AST change.

Feb 2 2023, 12:45 PM · Restricted Project, Restricted Project

Feb 1 2023

steven_wu committed rG5cff68fca0bc: [Module] Respect `-fno-pch-timestamps` when building modules (authored by steven_wu).
[Module] Respect `-fno-pch-timestamps` when building modules
Feb 1 2023, 10:35 AM · Restricted Project, Restricted Project
steven_wu closed D141632: [Module] Respect `-fno-pch-timestamps` when building modules.
Feb 1 2023, 10:34 AM · Restricted Project, Restricted Project
steven_wu committed rG516e30175256: [NFC][Profile] Access profile through VirtualFileSystem (authored by steven_wu).
[NFC][Profile] Access profile through VirtualFileSystem
Feb 1 2023, 9:25 AM · Restricted Project, Restricted Project, Restricted Project
steven_wu closed D139052: [NFC][Profile] Access profile through VirtualFileSystem.
Feb 1 2023, 9:25 AM · Restricted Project, Restricted Project, Restricted Project

Jan 31 2023

steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

Sorry, I'm still not really following - OK, sounds like you're saying this test does fail at HEAD/without this patch in reverse iteration mode, and is a bit larger than would be minimally necessary to achieve that, but also might fail at HEAD without reverse iteration, providing somewhat more testing than if it were fully minimized/only caught in reverse.

Fair enough -I don't think it's the right tradeoff, but I'm glad it's stable/provides that coverage.

Jan 31 2023, 4:45 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D139052: [NFC][Profile] Access profile through VirtualFileSystem.

Try fix pre-merge build failure

Jan 31 2023, 4:16 PM · Restricted Project, Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

No, reverse iteration will not break diff test for a small number of decls. Everything will be in reverse order so it is the same.

Hmm, I'm not sure I'm following why that is - could you explain this in more detail? The usual problem is that, say, we're outputting based on an unstable order - even two items would be enough to cause a test of that to fail in either forward or reverse iteration mode until the order is stabilized.

Is that not the case here? Is there some interaction between iteration order that's part of the nondeterminism here?

In order to make a test that will break before the change with reverse iteration, the test needs to check that the decls/records are serialized into the module in a pre deterministic order. It can't be just diff the output of two runs with a small input because small input will not overflow the smallptrset, thus the reverse iteration outputs from two runs will very likely to be identical, just in a different order from forward iteration.

Sure, I think I'm with you there - but the current test checks that the decls/records are serialized into the module in a pre-deterministic order, right? So it doesn't seem like a reverse iteration-failing test would be more involved/brittle/less robust than the test being added here?

Jan 31 2023, 3:12 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D139052: [NFC][Profile] Access profile through VirtualFileSystem.

Rebase to main.

Jan 31 2023, 1:46 PM · Restricted Project, Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

No, reverse iteration will not break diff test for a small number of decls. Everything will be in reverse order so it is the same.

Hmm, I'm not sure I'm following why that is - could you explain this in more detail? The usual problem is that, say, we're outputting based on an unstable order - even two items would be enough to cause a test of that to fail in either forward or reverse iteration mode until the order is stabilized.

Is that not the case here? Is there some interaction between iteration order that's part of the nondeterminism here?

Jan 31 2023, 1:40 PM · Restricted Project, Restricted Project

Jan 26 2023

steven_wu added a comment to D139816: [LTO] Don't generate invalid modules if "LTOPostLink" MD already exists.

I think this violates a core tenet of being a modular, reusable IR. It shouldn't be wrong to run a pass twice. It certainly shouldn't error by way of verifier error

Jan 26 2023, 11:40 AM · Restricted Project, Restricted Project
steven_wu added a comment to D139816: [LTO] Don't generate invalid modules if "LTOPostLink" MD already exists.

I am not the expert to answer that question. I will think this is an assertion-like behavior. It tells you as a compiler developer that your pipeline configuration is wrong in a sense that you should add all the functions in before running post link passes. You should delay the post link pass when you hit this error, rather than disable the check without a compelling reason.

Jan 26 2023, 11:23 AM · Restricted Project, Restricted Project

Jan 25 2023

steven_wu added a comment to D139816: [LTO] Don't generate invalid modules if "LTOPostLink" MD already exists.

My understanding of the flag is preventing you from:

  • run the devirtual pass
  • add some new function
  • run the devirtual pass again
Jan 25 2023, 10:29 AM · Restricted Project, Restricted Project

Jan 23 2023

steven_wu added a comment to D142318: [Support] Add PerThreadBumpPtrAllocator class..

I agree that this probably doesn't work for CAS since the use of CAS is not bounded to any context like a ThreadPoolExecutor, for example, it is currently a legal use case to have multiple thread pool to insert into CAS at the same time. It is not feasible to put such a restriction on CAS since CAS should be safe to read/write concurrently from different process.

Jan 23 2023, 10:14 AM · Restricted Project, Restricted Project

Jan 20 2023

steven_wu added a comment to D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified.

In my situation, at least, I am the vendor of the toolchain and my configuration file contains --sysroot=<CFGDIR>/../path/to/sysroot with a known relative path to where the sysroot is distributed. Apple is unique in this situation, since I am not distributing the sysroot, so there is no equivalent to using the <CFGDIR> variable. There is no way to invoke xcrun or set SDKROOT from config files, only pass flags.

It is true that the build system could do it, but I think it's reasonable for the driver to make a guess when not provided. I don't think it's all that different than detection of libstdc++ from GCC installs.

Jan 20 2023, 1:25 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D133504: Support: Add vfs::OutputBackend and OutputFile to virtualize compiler outputs.

Ping. Rebase the file on main

Jan 20 2023, 11:20 AM · Restricted Project, Restricted Project
steven_wu added a comment to D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified.

This is not an apple platform problem. This is a cross compile problem where you have a SDK that is installed for cross compile target. In those cases, you just can't hard code the path to isysroot, and figuring out the sysroot is generally considered a problem for the build system, not the problem for compiler. If you have a config file, you can definitely setup SDKROOT in your configuration system after running xcrun. A similar but not identical problem is if you just install clang package but didn't install dev packages on a linux platform where you just can't find any headers. Same error, just different causes.

Jan 20 2023, 11:12 AM · Restricted Project, Restricted Project
steven_wu added a comment to D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified.

That makes more sense, I thought perhaps it was using DEFAULT_SYSROOT. The shim isn't smart enough to choose the sysroot from the target unfortunately.

It looks like the only error is unrelated to this change, something with concepts in libc++.

Jan 20 2023, 10:19 AM · Restricted Project, Restricted Project
steven_wu added a comment to D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified.

One thing to throw into the mix: Apple's clang has a default sysroot configured, so with the default system compiler, there is no way to replicate this "build without a sysroot" scenario as far as I can tell. For the system compiler, I believe this behavior is a strict improvement.

Jan 20 2023, 9:38 AM · Restricted Project, Restricted Project

Jan 19 2023

steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

No, reverse iteration will not break diff test for a small number of decls. Everything will be in reverse order so it is the same. Current test will fail early in reverse iteration and will fail in the end statistically for forward iteration. Will pass in all configurations. I think paying a bit more decls to make it fail in forward iteration is not a bad trade off.

Jan 19 2023, 1:04 PM · Restricted Project, Restricted Project
steven_wu added a comment to D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified.

This breaks macOS bot too: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/33839/

Jan 19 2023, 12:57 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D141625: [DeclContext] Sort the Decls before adding into DeclContext.

Update tests according to feedback

Jan 19 2023, 11:35 AM · Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

@dblaikie Do you have any objection for committing the patch as it is? I don't think reverse iteration test is a proper way to test for this specific bug.

I think reverse iteration is the right way to test this specific bug (& any bug where behavior may be unintentionally nondeterministic due to non-deterministically ordered containers) - it makes the testing more reliable rather than depending on implementation details of such containers that aren't guaranteed (that being the point/problem).

But it's not the worst/most unwieldy test for this sort of thing & if we don't have bots using it... hmm, actually I looked more closely & maybe we do have reverse iteration builders: https://github.com/llvm/llvm-zorg/search?q=reverse-iteration (though I'm having trouble navigating the builder UI to see whether there are up-to-date builds for this configuration)

Could you help me understand your perspective regarding using reverse iteration to test this specific bug? (are there some bugs you find reverse iteration suitable for? what's different about this one from them?)

Jan 19 2023, 11:19 AM · Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

@dblaikie Do you have any objection for committing the patch as it is? I don't think reverse iteration test is a proper way to test for this specific bug.

Jan 19 2023, 10:07 AM · Restricted Project, Restricted Project

Jan 17 2023

steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

@dblaikie Do we have any bots running reverse iteration?

Hmm, not that I can see/find at a quick glance, which is unfortunate. @mgrang Are you still working on LLVM/have any knowledge of reverse iteration testing being done? (@colinl - looks like maybe you're a Code Aurora person, perhaps you've got some more recent context for this?)

Jan 17 2023, 3:53 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D133716: [CAS] Add LLVMCAS library with InMemoryCAS implementation.

Update after makeArrayRef depreciation.

Jan 17 2023, 11:32 AM · Restricted Project, Restricted Project
steven_wu updated the diff for D133713: [Support] Introduce ThreadSafeAllocator.

Formatting and minor update.

Jan 17 2023, 11:32 AM · Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

EXPANSIVE_CHECKS will reshuffle the llvm::sort input: https://lists.llvm.org/pipermail/llvm-dev/2018-April/122576.html

Jan 17 2023, 11:25 AM · Restricted Project, Restricted Project
steven_wu updated the diff for D133713: [Support] Introduce ThreadSafeAllocator.

Address review feedback

Jan 17 2023, 9:41 AM · Restricted Project, Restricted Project
steven_wu updated the diff for D133715: [ADT] Add TrieRawHashMap.

Update after makeArrayRef deprecation

Jan 17 2023, 9:40 AM · Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

@dblaikie Do we have any bots running reverse iteration?

Jan 17 2023, 9:10 AM · Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

Actually, sorting in numberAnonymousDeclsWithin doesn't work for some reasons.

Jan 17 2023, 9:01 AM · Restricted Project, Restricted Project

Jan 16 2023

steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

Actually, sorting in numberAnonymousDeclsWithin doesn't work for some reasons.

Jan 16 2023, 9:56 AM · Restricted Project, Restricted Project
steven_wu updated the diff for D141625: [DeclContext] Sort the Decls before adding into DeclContext.

Minor touch up on the test case. Also add some comments in test to explain what is being tested.

Jan 16 2023, 9:33 AM · Restricted Project, Restricted Project

Jan 13 2023

steven_wu updated the diff for D141625: [DeclContext] Sort the Decls before adding into DeclContext.

@akyrtzi has the good idea. It is really hard to control Decl* to get values
to get an unstable iteration order from the small tests, going beyond 32 decls
to get out of SmallPtrSet's small model is much consistent.

Jan 13 2023, 4:19 PM · Restricted Project, Restricted Project
steven_wu added a comment to D141625: [DeclContext] Sort the Decls before adding into DeclContext.

Yeah, might be nice to document where the instability comes from - if it comes from a DenseMap or similar, then a test that fails either in forward or reverse iteration mode would be nice to have.

Scope::decls is iterating over a SmallPtrSet. This might be a little annoying to create a test for, because it'll only be unstable if we have two decls in the same prototype scope that get allocated into different slabs by the bump ptr allocator, but maybe something like:

void f(struct A *p, int arr[1 + 0 + 0 + 0 + ... + 0], struct B *q);

... would be enough (with sufficient subexpressions to fill a whole slab). Then we can build a .pch for that twice and check that it comes out identical.

Jan 13 2023, 3:38 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D133715: [ADT] Add TrieRawHashMap.

Update unittest:

Jan 13 2023, 12:04 PM · Restricted Project, Restricted Project
steven_wu added inline comments to D133715: [ADT] Add TrieRawHashMap.
Jan 13 2023, 10:00 AM · Restricted Project, Restricted Project

Jan 12 2023

steven_wu requested review of D141632: [Module] Respect `-fno-pch-timestamps` when building modules.
Jan 12 2023, 1:47 PM · Restricted Project, Restricted Project
steven_wu requested review of D141625: [DeclContext] Sort the Decls before adding into DeclContext.
Jan 12 2023, 11:37 AM · Restricted Project, Restricted Project

Jan 10 2023

steven_wu added inline comments to D133715: [ADT] Add TrieRawHashMap.
Jan 10 2023, 2:11 PM · Restricted Project, Restricted Project

Jan 9 2023

steven_wu updated the diff for D139052: [NFC][Profile] Access profile through VirtualFileSystem.

Address review feedback. Remove NFC from title.

Jan 9 2023, 4:16 PM · Restricted Project, Restricted Project, Restricted Project
steven_wu updated the diff for D133716: [CAS] Add LLVMCAS library with InMemoryCAS implementation.

Update patch after HashMap rename

Jan 9 2023, 10:14 AM · Restricted Project, Restricted Project
steven_wu retitled D133715: [ADT] Add TrieRawHashMap from [ADT] Add HashMappedTrie to [ADT] Add TrieRawHashMap.
Jan 9 2023, 9:56 AM · Restricted Project, Restricted Project
steven_wu updated the diff for D133715: [ADT] Add TrieRawHashMap.

Rename to TrieRawHashMap.

Jan 9 2023, 9:55 AM · Restricted Project, Restricted Project

Jan 6 2023

steven_wu added a comment to D133715: [ADT] Add TrieRawHashMap.

Maybe RawHashTrieMap? It reads better when Raw is in the front, and it contains hash-trie and trie-map, which are both terms describing data structures similar to this but this is much simpler, thus raw.

Jan 6 2023, 3:58 PM · Restricted Project, Restricted Project
steven_wu added a comment to D133715: [ADT] Add TrieRawHashMap.

@dexonsmith @benlangmuir @akyrtzi
Does TrieHashMap sound good to you?

Jan 6 2023, 2:24 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D133714: [ADT] Introduce LazyAtomicPointer.

Minor format and test update

Jan 6 2023, 10:43 AM · Restricted Project, Restricted Project
steven_wu updated the diff for D133713: [Support] Introduce ThreadSafeAllocator.

Update unittest to wait and check.

Jan 6 2023, 10:43 AM · Restricted Project, Restricted Project
steven_wu added a comment to D133713: [Support] Introduce ThreadSafeAllocator.

@dexonsmith Move the discussion here.

Jan 6 2023, 9:16 AM · Restricted Project, Restricted Project
steven_wu added a comment to D133715: [ADT] Add TrieRawHashMap.

But having a fast concurrent BumpPtrAllocator would be independently useful, and I'd suggest optimizing the allocator before bloating the default trie size.

+1 for fast concurrent ThreadSafeBumpPtrAllocator.

What do you think about following alternative implementation?

class ThreadSafeBumpPtrAllocator {
  ThreadSafeBumpPtrAllocator() {
    size_t ThreadsNum = ThreadPoolStrategy.compute_thread_count();
    allocators.resize(ThreadsNum);
  }
  
  void* Allocate (size_t Num) {
      size_t AllocatorIdx = getThreadIdx();
      
      return allocators[AllocatorIdx].Allocate(Num);
  }

  std::vector<BumpPtrAllocator> allocators;
};

static thread_local ThreadIdx;

size_t getThreadIdx() {
  return ThreadIdx;
}

This implementation uses the fact that ThreadPoolExecutor creates a fixed number
of threads(ThreadPoolStrategy.compute_thread_count()) and keeps them until destructed
. ThreadPoolExecutor can initialise thread local field ThreadIdx to the proper thread index.
The getThreadIdx() could return index of thread inside ThreadPoolExecutor.Threads.
ThreadSafeBumpPtrAllocator keeps separate allocator for each thread. In this case each thread would
always use separate allocator. No neccessary to have locks, cas operations, no races...

Jan 6 2023, 9:03 AM · Restricted Project, Restricted Project

Jan 5 2023

steven_wu updated the diff for D133716: [CAS] Add LLVMCAS library with InMemoryCAS implementation.

Add docs in the reference page that address CAS in different prespective.

Jan 5 2023, 3:32 PM · Restricted Project, Restricted Project
steven_wu added a comment to D133715: [ADT] Add TrieRawHashMap.

Ping. All feedbacks are addressed.

Additional notes: I dig a bit deeper into the benchmark from https://reviews.llvm.org/D132548 as it shows bad scaling during parallel insertion tests (stop scaling to 8 threads). That is actually caused by our ThreadSafeBumpPtrAllocator that currently takes a lock. We can improve it in future since our use case doesn't expect heavy parallel insertions. However, it is quite obvious we should tune to RootBits and SubTrieBits. Increasing RootBits can significantly decrease the contention. A better strategy might be something like starting with something like 10 bits, then 4 bits, 2 bits and 1 bit. Shrinking number of bits can lead to better memory usage since the slots usage in the deep nodes are very low.

Could ThreadSafeBumpPtrAllocator could be made lock-free? I think at least it would be possible to implement one that only locked when a new allocation is needed, instead of every time the ptr is bumped as now. (I’ll think about it a bit.)

Note that in the CAS use case it’s ideally true that most insertions are duplicates and don’t need to call the allocator at all. This is why we’ve been able to get away with a lock on each allocation.

Jan 5 2023, 2:11 PM · Restricted Project, Restricted Project
steven_wu added a comment to D133715: [ADT] Add TrieRawHashMap.

The ability to dump the data structure I'm all for - like DenseMap, etc, but I think that dumping should be the user-visible state (it's a map, the trie part is an implementation detail that ideally shouldn't be exposed in the dump developers use regularly - presumably it's not necessary except when debugging the data structure itself, which should be rare (we don't usually need to look at DenseMap's internal bucketing, for instance)).

Jan 5 2023, 2:01 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D133713: [Support] Introduce ThreadSafeAllocator.

Fix non-assert build test

Jan 5 2023, 12:19 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D133713: [Support] Introduce ThreadSafeAllocator.

Address review feedback

Jan 5 2023, 11:39 AM · Restricted Project, Restricted Project
steven_wu added a comment to D133715: [ADT] Add TrieRawHashMap.

Ping. All feedbacks are addressed.

Jan 5 2023, 11:12 AM · Restricted Project, Restricted Project
steven_wu updated the diff for D133715: [ADT] Add TrieRawHashMap.

Allow checking prefix of the sub-trie

Jan 5 2023, 10:53 AM · Restricted Project, Restricted Project

Jan 4 2023

steven_wu updated the diff for D133715: [ADT] Add TrieRawHashMap.

Fix typo in the comments

Jan 4 2023, 3:32 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D133715: [ADT] Add TrieRawHashMap.

Update tests to avoid string comparsion.

Jan 4 2023, 3:30 PM · Restricted Project, Restricted Project
steven_wu added a comment to D132455: [ADT] add ConcurrentHashtable class..

I am super interested in how you do the comparison. Can you post a patch for the code you have for that? The HashMappedTrie hasn't been really tuned for performance/memory usage, it would be interesting to use your tool to do some investigation (for example, the NumRootBits and NumSubtrieBits can be tuned for memory/performance).

@steven_wu I`ve updated D132548 to have a possibility to measure HashMappedTrie. It depends on D125979, D132455, D133715.
In its current state patch does not set NumRootBits, NumSubtrieBits, though such options might be added.
If intel threading building block hashmap is not neccessary - the USE_ITBB should be unset.
if lib cuckoo hashmap is not neccessary - the USE_LIBCUCKOO should be unset.
Command line to run the tool:

/usr/bin/time -f " %E %M " ./check-hashtable --data-set random --num-threads 1 --table-kind hash-mapped-trie --aggregate-data

Any changes/suggestions are welcomed :-)

Jan 4 2023, 12:12 PM · Restricted Project, Restricted Project

Jan 3 2023

steven_wu updated the diff for D133715: [ADT] Add TrieRawHashMap.

Add some documentation about HashMappedTrie

Jan 3 2023, 2:57 PM · Restricted Project, Restricted Project
steven_wu added inline comments to D133715: [ADT] Add TrieRawHashMap.
Jan 3 2023, 1:01 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D133716: [CAS] Add LLVMCAS library with InMemoryCAS implementation.

Rebase for std::optional

Jan 3 2023, 12:43 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D133715: [ADT] Add TrieRawHashMap.

Address review feedback and rebase the patch using std::optional

Jan 3 2023, 12:42 PM · Restricted Project, Restricted Project
steven_wu added a comment to D133715: [ADT] Add TrieRawHashMap.

eh, maybe string's easier to read in expect diagnostics anyway... just seems a bit awkward/circuitous/unfortunate :/ (I guess the stringification could move into the test code, implemented via such a friend relationship)

I do remember the string being easy to read in expect diagnostics, FWIW.

What about renaming the methods to printLayout() and dumpLayout()? Then print() and dump() would at least still be available for something user-centric.

Workable, though I'd still rather it be moved to the test file if it's not too inconvenient (with some friendship, probably). Avoid muddying the implementation with test-only features.

Jan 3 2023, 10:42 AM · Restricted Project, Restricted Project
steven_wu added a comment to D132455: [ADT] add ConcurrentHashtable class..

Thanks for doing the comparison with https://reviews.llvm.org/D133715.

Jan 3 2023, 10:15 AM · Restricted Project, Restricted Project

Dec 22 2022

steven_wu added inline comments to D140420: [Support] Update modulemap for TargetParser.
Dec 22 2022, 2:57 PM · Restricted Project, Restricted Project

Dec 21 2022

steven_wu added inline comments to D140420: [Support] Update modulemap for TargetParser.
Dec 21 2022, 9:37 AM · Restricted Project, Restricted Project

Dec 20 2022

steven_wu added inline comments to D140420: [Support] Update modulemap for TargetParser.
Dec 20 2022, 3:07 PM · Restricted Project, Restricted Project
steven_wu added inline comments to D140420: [Support] Update modulemap for TargetParser.
Dec 20 2022, 12:56 PM · Restricted Project, Restricted Project
steven_wu added a comment to D139938: [clang] Re-apply change to avoid passing -stdlib=libc++ spuriously to CC1 on Darwin.

This seems to have broken the instrprof-darwin-exports.c test, see e.g. https://green.lab.llvm.org/green/job/clang-stage1-RA/32351/

I'll revert for now.

Dec 20 2022, 12:36 PM · Restricted Project, Restricted Project, Restricted Project
steven_wu added inline comments to D140247: Disable Univariate3D_Invert to workaround module build failure.
Dec 20 2022, 10:49 AM · Restricted Project, Restricted Project
steven_wu added a comment to D137838: [Support] Move TargetParsers to new component.

Also needed a follow up fix to completely fix module 9cd6fbee7ed8

Dec 20 2022, 10:32 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
steven_wu committed rG9cd6fbee7ed8: Fix module build after TargetParser (authored by steven_wu).
Fix module build after TargetParser
Dec 20 2022, 10:31 AM · Restricted Project, Restricted Project, Restricted Project

Dec 19 2022

steven_wu added inline comments to D139924: LTO C API: always parse modules in opaque pointer mode..
Dec 19 2022, 4:03 PM · Restricted Project, Restricted Project
steven_wu added a reverting change for rG8ba9a5218782: LTO: always parse modules in opaque pointer mode.: rG3a5426f57243: Revert "LTO: always parse modules in opaque pointer mode.".
Dec 19 2022, 4:03 PM · Restricted Project, Restricted Project
steven_wu committed rG3a5426f57243: Revert "LTO: always parse modules in opaque pointer mode." (authored by steven_wu).
Revert "LTO: always parse modules in opaque pointer mode."
Dec 19 2022, 4:03 PM · Restricted Project, Restricted Project

Dec 16 2022

steven_wu updated the diff for D140247: Disable Univariate3D_Invert to workaround module build failure.

Fix typo

Dec 16 2022, 2:13 PM · Restricted Project, Restricted Project
steven_wu requested review of D140247: Disable Univariate3D_Invert to workaround module build failure.
Dec 16 2022, 2:07 PM · Restricted Project, Restricted Project

Dec 15 2022

steven_wu accepted D140164: [clang/Lexer] Enhance `Lexer::getImmediateMacroNameForDiagnostics` to return a result from non-file buffers.

LGTM

Dec 15 2022, 3:12 PM · Restricted Project, Restricted Project

Dec 14 2022

steven_wu added a comment to D139905: [NFC] Cleanup: BasicBlock::getInstList() is now private..

This breaks llvm/example:

/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/examples/ModuleMaker/ModuleMaker.cpp:58:7: error: 'getInstList' is a private member of 'llvm::BasicBlock'

See: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/33265/console

Dec 14 2022, 1:19 PM · Restricted Project, Restricted Project
steven_wu added a comment to D133715: [ADT] Add TrieRawHashMap.
  • a test support friend class that can inspect/test the trie layout without relying on stringification;

This bit I'm not so sure about - we don't test the bucketing behavior of DenseHashMap so far as I know (or other similar implementation details of the various other hashing data structures - (since they're the ones with fairly complicated implementation details, they seem the best comparison)), for instance, we only test its interface - why would we do differently for this data structure?

IMO the layout is more complex than a flat array with quadratic probing. The logic for equal and/or nearly-equal hashes is a bit subtle, and there were hard-to-reason about bugs originally. The layout-stringification was necessary to understand what was going wrong, and the tests that use it help ensure future refactorings don't get it wrong.

I don't remember the bugs, but two examples of subtleties:

  • On an exact match you don't want to "sink" the existing entry down a level to a new sub-trie (you need to detect "exact match" before sinking). Getting this wrong will affect performance but not otherwise be user-visible.
  • The deepest sub-trie might be a different/smaller size than the others because there are only a few bits left-over, and the handling needs to be right. It's simpler to check for correct layout directly than to guess about what user-visible effects there might be for errors.

I'd be a bit uneasy with the layout tests being dropped altogether.

Maybe an alternative to testing the layout directly would be to add a verification member function that iterated through the data structure and ensured everything was self-consistent (else crash? else return false?). Then the tests could call the member function after a series of insertions that might trigger a "bad" layout.

Fair enough - if it's sufficient to have a verify operation (maybe "assertValid" - so, yeah, crash when not valid) I'd go with that, but given the argument you've made, if you think verifying the specific structure is significantly more valuable than that, I'd be OK with some private/test-friended introspection API.

IMO it's worth it; @steven_wu, if you disagree, I could live with assertValid(). (BTW, I remembered another justification. The test case inputs/setups are a bit subtle; checking the layout ensures you've covered the corner case you think you've covered.)

Dec 14 2022, 12:42 PM · Restricted Project, Restricted Project
steven_wu added a comment to D131858: [clang] Track the templated entity in type substitution..

The workaround in https://reviews.llvm.org/D139956 can get llvm-project bootstrap correctly, doesn't fix the underlying issue.

Dec 14 2022, 11:19 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project

Dec 13 2022

steven_wu committed rGe77e14ecf17b: Workaround an assertion failure during module build (authored by steven_wu).
Workaround an assertion failure during module build
Dec 13 2022, 10:43 AM · Restricted Project, Restricted Project
steven_wu closed D139956: Workaround an assertion failure during module build.
Dec 13 2022, 10:42 AM · Restricted Project, Restricted Project
steven_wu added a reviewer for D139956: Workaround an assertion failure during module build: lhames.
Dec 13 2022, 10:41 AM · Restricted Project, Restricted Project
steven_wu requested review of D139956: Workaround an assertion failure during module build.
Dec 13 2022, 10:37 AM · Restricted Project, Restricted Project
steven_wu accepted D139924: LTO C API: always parse modules in opaque pointer mode..

LGTM

Dec 13 2022, 6:08 AM · Restricted Project, Restricted Project