- User Since
- Apr 30 2013, 5:34 PM (224 w, 6 d)
Sun, Aug 20
Thu, Aug 17
Bi-weekly ping! (@rsmith)
Mon, Aug 14
Seems fine to me. Adding @dexonsmith (I don't know who maintain libc++ at Apple right now)
Wed, Aug 9
Is this something that is affecting clang-5.0 backward compatibility with clang-4.0?
(adding @hansw for visibility)
Tue, Aug 8
Mon, Aug 7
Sat, Aug 5
Fri, Aug 4
Address Richard's comment
Thu, Aug 3
I'll leave the final approval to Teresa.
When the working set size is determined to be huge, disable peeling to avoid bloating the working set further.
Ping again @rsmith (or anyone else)
Wed, Aug 2
Mon, Jul 31
Sun, Jul 30
It seems to me to conceptually belong to the backend. Why isn't this part of CodeGenPrepare (or injected by the target as part of its pre-ISel IR passes)?
Wed, Jul 26
Tue, Jul 25
they had to be emitted linkonce_odr in all the destination modules (even if they weren't used by an alias) rather than as available_externally - causing extra object size.
Mon, Jul 24
Weekly ping! (@rsmith)
Here is the current output:
Jul 21 2017
Jul 20 2017
Thanks, that's a very good start! Few comments inline, I didn't understand everything.
Thanks, I'm glad to see this coming!
Oh great you just committed :) We were on the same line! I just wanted to make sure you were not waiting for me. Thanks.
Note: I approved in the first place because my comment was so minor that you could fix it (or not) and move on with committing directly without another round of review.
Jul 17 2017
@rsmith: post-C++-commitee-meeting ping ;)
Jul 14 2017
Jul 13 2017
Thanks, very clear :)
Teresa: can you describe a bit more how we end up importing two summaries for the same GUID? I would expect that after importing one, we don't even come to try to import another since we already have one.
Jul 12 2017
Jul 11 2017
With @sanjoy approval of langref, LGTM.
Jul 10 2017
Jul 9 2017
Missing test showing the import happening on critical and not without. Right now the test you provided only shows that the "critical" flag is encoded in the binary. The change in the function importer is not tested, unless I missed something.
The context and the motivation is *still* not clear to me. To begin why do we need the *names* when doing the thin link?
As Teresa mentioned, please abandon this revision and recreate a new one.
Fix issues around mutable fields and regression on "internal", add more testing.
When we used an std::map originally it was because we needed the ordering. Have you checked that we don't iterate on any instance of this map?
Jul 7 2017
Please add the motivation/context for this patch, and a test. Thanks!
Jul 6 2017
@davidxl: while not a bug, it is still annoying from an internal design point of view to tie our behavior to the hash function used in StringMap. Conceptually they are not supposed to be ordered!
I had a similar issue when I tried to change the StringMap hash function in LLVM and it broke a bunch of test because LLVM's behavior changes depending on the order of iteration there (this is deterministic but the hash function is part of the inputs to consider for determinism basically...)
Jul 5 2017
Make sure your commit message correctly reference the LLVM change.
Jul 4 2017
Jun 30 2017
Jun 29 2017
I'll be happy to iterate separately on providing an easy way to build a boostrap of clang (that would be easy to use with or without Docker)!
Jun 28 2017
Jun 27 2017
(maybe it is already?)
Should this be documented in the LLVM LangRef? (or elsewhere?)
(Otherwise I'd expect some documentation on the `upgradeThumbMode``` function at least)
Jun 26 2017
The description says "The motivation example is a fully unrolled 3x3 matrix multiplication. It loads every data in matrix a and b three times because of the Stores between them", but from reading the code I don't see how this case is handled since it seems that you bail as soon as you see a store (or a call)? What did I miss? Is the description still up-to-date?
Also, please upload patches with full context (git diff -U999999)
This seems quite intrusive and adding a quite some complexity inline (and increasing the invariant surface for BitcodeWriterBase apparently).
I'm not convinced that this is the right approach at this point: we're really stretching the `WriteBitcodeToFile``` API quite far.