Page MenuHomePhabricator

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 (333 w, 18 h)

Recent Activity

Tue, Jul 21

dexonsmith added a reviewer for D82118: [clang][module] Improve incomplete-umbrella warning: Bigcheese.
Tue, Jul 21, 11:00 AM · Restricted Project

Mon, Jul 20

dexonsmith accepted D84205: [compiler-rt] Use -nostdinc++ in clang_rt.profile to avoid including C++ headers.

LGTM.

Mon, Jul 20, 9:16 PM · Restricted Project

Tue, Jul 14

dexonsmith committed rG6c7419ecc565: utils: Tweak clang-parse-diagnostics-file for modules includes (authored by dexonsmith).
utils: Tweak clang-parse-diagnostics-file for modules includes
Tue, Jul 14, 4:38 PM
dexonsmith committed rG3eaa69b395b5: clang: Quash an instance of -Wdeprecated-copy, NFC (authored by dexonsmith).
clang: Quash an instance of -Wdeprecated-copy, NFC
Tue, Jul 14, 4:16 PM
dexonsmith committed rG4c494013b7bd: clang: Quash some -Wrange-loop-analysis warnings, NFC (authored by dexonsmith).
clang: Quash some -Wrange-loop-analysis warnings, NFC
Tue, Jul 14, 4:16 PM
dexonsmith committed rG30b1792181e0: clang: Only define OBJC_NEW_PROPERTIES when -x objective-c (authored by dexonsmith).
clang: Only define OBJC_NEW_PROPERTIES when -x objective-c
Tue, Jul 14, 4:16 PM

Jul 2 2020

dexonsmith requested changes to D83071: Add support for options with two flags for controlling the same field..
Jul 2 2020, 7:25 PM · Restricted Project, Restricted Project

Jun 26 2020

dexonsmith resigned from D6390: Ensure debug info for two calls to the same function from the same location are not merged.
Jun 26 2020, 3:12 PM
dexonsmith resigned from D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers.
Jun 26 2020, 3:11 PM · Restricted Project
dexonsmith requested changes to D82661: [clang-tidy][NFC] Remove unnecessary includes throughout clang-tidy header files.

Plus replacing a few std::map<string,...> with llvm::StringMap

Jun 26 2020, 3:10 PM · Restricted Project

Jun 25 2020

dexonsmith accepted D82610: [compiler-rt] Add support for arm64 macOS.

LGTM, after you fix the nits I mention inline.

Jun 25 2020, 4:56 PM · Restricted Project
dexonsmith accepted D82514: Remove references to the 4.0 release as a major breaking (NFC).

This LGTM, assuming @echristo is happy.

Jun 25 2020, 2:44 PM · Restricted Project

Jun 24 2020

dexonsmith added a reviewer for D81905: Enhance Itanium demangler interface.: hfinkel.

Yes, I infer semantics properties. I use demangler to implement lowering of "C++ intrinsics" - Itanium-mangled C++ name (e.g. templated function instantiation) - for my SPIRV-based target. Lowering depends on the name, template parameter values and argument types (if overloaded). https://github.com/intel/llvm/pull/1881

If its to infer some semantic property of the symbol then there probably are better ways of expressing that, like getting the frontend to add metadata or a function attribute or something.

It is not obvious to me that metadata + attributes is better than demangling (as long as it implemented correctly) - with demangling implementation is more self-encapsulated and simpler. But that's pretty much orthogonal to this code change.

Jun 24 2020, 3:47 PM · Restricted Project
dexonsmith resigned from D72550: [SCCIterator] Fix another potential use-after-free.
Jun 24 2020, 3:47 PM · Restricted Project
dexonsmith resigned from D70628: [Support] Enable file + line info in LLVM stack traces on Darwin..
Jun 24 2020, 3:47 PM · Restricted Project
dexonsmith requested changes to D73422: [IR] Delete MODULE_CODE_DEPLIB.

Anything that was "planned to be removed in 4.0" is now supported "forever" until we make an explicit choice to break backward compatibility.

Jun 24 2020, 3:47 PM · Restricted Project
dexonsmith resigned from D82233: [lit] Add --show-xxx command line options.
Jun 24 2020, 3:13 PM · Restricted Project

Jun 22 2020

dexonsmith accepted D82337: [Triple] support macOS 11 os version number.

LGTM.

Jun 22 2020, 5:12 PM · Restricted Project

Jun 15 2020

dexonsmith resigned from D80274: [MachineVerifier] Handle the PHI node for verifyLiveVariables().
Jun 15 2020, 10:55 AM · Restricted Project

Jun 12 2020

dexonsmith added inline comments to D79796: Sketch support for generating CC1 command line from CompilerInvocation.
Jun 12 2020, 6:15 PM · Restricted Project, Restricted Project

Jun 10 2020

dexonsmith accepted D80383: Add AST_SIGNATURE record to unhashed control block of PCM files.

LGTM, with one more comment inline.

Jun 10 2020, 12:46 PM · Restricted Project
dexonsmith added inline comments to D79796: Sketch support for generating CC1 command line from CompilerInvocation.
Jun 10 2020, 12:21 PM · Restricted Project, Restricted Project

Jun 8 2020

dexonsmith accepted D81329: ADT: Fix that APSInt's string constructor claims it requires 5 bits to store a zero.

LGTM.

Jun 8 2020, 1:51 PM · Restricted Project
dexonsmith added inline comments to D79796: Sketch support for generating CC1 command line from CompilerInvocation.
Jun 8 2020, 1:51 PM · Restricted Project, Restricted Project
dexonsmith accepted D81347: Make ASTFileSignature an array of 20 uint8_t instead of 5 uint32_t.

LGTM once @aprantl is happy.

Jun 8 2020, 10:29 AM · Restricted Project

Jun 5 2020

dexonsmith added a comment to D79796: Sketch support for generating CC1 command line from CompilerInvocation.

I have a couple of drive-by suggestions, up to @dang and @Bigcheese whether to incorporate them.

Jun 5 2020, 12:22 PM · Restricted Project, Restricted Project

Jun 4 2020

dexonsmith accepted D80849: [apple clang] disable in-process CC1 to preserve crashlog compatibility.

LGTM, but next we should get the crash reproductions working with in-process -cc1.

Jun 4 2020, 4:03 PM · Restricted Project

Jun 3 2020

dexonsmith added a comment to D80383: Add AST_SIGNATURE record to unhashed control block of PCM files.

I made the decision to make all relative offsets relative to the start of the AST block rather than their own sub-block or a neighboring block, in order to prevent adding complexity in ASTReader. My reasoning that it would be best if the reader didn't have to keep track of a bunch of offsets inside the bitcode at all times, but that it could just remember the offset of the AST block once and always use that for relative offsets. I agree that conceptually making the sub-blocks relocatable too would have been nice, but I figured it wasn't worth the extra complexity in the reader.

Jun 3 2020, 12:38 PM · Restricted Project

Jun 1 2020

dexonsmith requested changes to D80383: Add AST_SIGNATURE record to unhashed control block of PCM files.

Thanks for working on this! I have a number of suggestions inline.

Jun 1 2020, 3:11 PM · Restricted Project
dexonsmith added a comment to D80808: Add DIAError.h to list of headers excluded from the LLVM_DebugInfo_PDB module.

Can you commit this for me?

Jun 1 2020, 10:46 AM · Restricted Project

May 29 2020

dexonsmith accepted D80808: Add DIAError.h to list of headers excluded from the LLVM_DebugInfo_PDB module.

LGTM.

May 29 2020, 10:23 AM · Restricted Project

May 21 2020

dexonsmith added inline comments to D80052: [docs] Sketch outline for HowToUpdateDebugInfo.rst.
May 21 2020, 3:43 PM · debug-info, Restricted Project

May 18 2020

dexonsmith added a comment to D79916: Map -O to -O1 instead of -O2.

IOW, this LGTM if Alex and Gerolf are happy.

May 18 2020, 3:46 PM · Restricted Project
dexonsmith added inline comments to D80055: Diagnose union tail padding.
May 18 2020, 2:39 PM · Restricted Project
dexonsmith added inline comments to D80055: Diagnose union tail padding.
May 18 2020, 11:54 AM · Restricted Project

May 15 2020

dexonsmith added inline comments to D80055: Diagnose union tail padding.
May 15 2020, 7:01 PM · Restricted Project
dexonsmith updated subscribers of D79916: Map -O to -O1 instead of -O2.

I'm totally down, but you knew that already :)

Duncan: Do you have any concerns? I doubt it, but just checking.

May 15 2020, 5:56 PM · Restricted Project
dexonsmith accepted D79916: Map -O to -O1 instead of -O2.

IOW, this LGTM if Alex and Gerolf are happy.

May 15 2020, 5:56 PM · Restricted Project
dexonsmith committed rG558db27c4971: [NFC] Whitespace fix inside OptParserEmitter (authored by dang).
[NFC] Whitespace fix inside OptParserEmitter
May 15 2020, 11:58 AM
dexonsmith closed D79989: [NFC] Whitespace fix inside OptParserEmitter.

Committed in 558db27c4971cedde1a5915f390a7897e3038e10.

May 15 2020, 11:57 AM · Restricted Project
dexonsmith accepted D79989: [NFC] Whitespace fix inside OptParserEmitter.

LGTM. I can commit for you.

May 15 2020, 11:25 AM · Restricted Project

May 14 2020

dexonsmith added inline comments to D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check.
May 14 2020, 1:03 PM · Restricted Project
dexonsmith added a comment to D74813: Function block naming - add hash of parameter type to end of block name.

@dexonsmith - Are you OK with that ?

May 14 2020, 1:03 PM · Restricted Project, Restricted Project

May 13 2020

dexonsmith added inline comments to D79796: Sketch support for generating CC1 command line from CompilerInvocation.
May 13 2020, 11:57 AM · Restricted Project, Restricted Project

May 7 2020

dexonsmith added a comment to D79343: [libc++][test] Adjust move_iterator tests to allow C++20.

Regarding the formatting changes, I personally think we should clang-format all of libc++, libc++abi and libunwind in one go. Then we'd be done with these small issues that add up. And we can even add the revision to .git-blame-ignore-revs.

May 7 2020, 5:23 PM · Restricted Project

May 6 2020

dexonsmith added a comment to D79343: [libc++][test] Adjust move_iterator tests to allow C++20.

I'm not going to reformat *only* my additions per the clang-format instructions - that would be silly - and I suspect that folks would clang-format all of the tests if they actually wanted them formatted, so I won't format the entire file, either.

May 6 2020, 3:19 PM · Restricted Project

Apr 30 2020

dexonsmith added a comment to D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

(seems a bit questionable to rely on uintptr_t being necessarily the same type as either uint32_t or uint64_t - but maybe that's guaranteed/written down somewhere)?

Apr 30 2020, 8:35 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D78816: Fix ostream for complex numbers with fixed field width.

@cantonios, thanks for working on this. I'll let @ldionne or others review in detail, but I have two high-level notes:

Apr 30 2020, 10:41 AM · Restricted Project

Apr 29 2020

dexonsmith added a comment to D74813: Function block naming - add hash of parameter type to end of block name.

@dexonsmith what regression are you referring to ?
What this change is effectively doing now is changing the numbering of the blocks from incremental to hash-based.
So the demangler functionality remains the same (i think) - I saw that it is ignoring the (currently incrememntal) number, so changing it shouldn't make a difference.
Did I miss anything ?

Apr 29 2020, 4:12 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D74813: Function block naming - add hash of parameter type to end of block name.

@erik.pilkington would the hash-based numbering be OK for now ?

Feel free to drop the demangler changes for now, but I would prefer including the output of the mangled type in the symbol rather than it's hash.

Apr 29 2020, 1:28 PM · Restricted Project, Restricted Project

Apr 24 2020

dexonsmith added a comment to D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

@nikic, great news! Thanks for doing the detailed analysis.

Apr 24 2020, 11:20 AM · Restricted Project, Restricted Project

Apr 23 2020

dexonsmith added a comment to D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

Okay, I'm basically fine with that, if it is our stance that SmallVector should always be preferred over std::vector. Not really related to this revision, but it would probably help to do some renaming/aliasing to facilitate that view. Right now, the number of SmallVector<T, 0> uses in LLVM is really small compared to the std::vector<T> uses (100 vs 6000 based on a not very accurate grep). I think part of that is in the name, and calling it using Vector<T> = SmallVector<T, 0> and using VectorImpl<T> = SmallVectorImpl<T> would make it a lot more obvious that this is our preferred general purpose vector type, even if the stored data is not small.

Apr 23 2020, 9:42 PM · Restricted Project, Restricted Project

Apr 22 2020

dexonsmith added a comment to D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

Thanks for the revert explanation and notes, nikic.

@dexonsmith what is your current thinking on going back to the original std::vector approach?

Apr 22 2020, 11:57 PM · Restricted Project, Restricted Project

Apr 21 2020

dexonsmith added a comment to D77601: Make SmallVector assert if it cannot grow..

Incorporated into https://reviews.llvm.org/D77621 (because this is not submitted yet, and that change now moves this code).

Apr 21 2020, 6:25 PM · Restricted Project

Apr 17 2020

dexonsmith updated subscribers of D78403: Infer alignment of loads with unspecified alignment in IR/bitcode parsing..
Apr 17 2020, 6:03 PM · Restricted Project
dexonsmith added a comment to D78370: [libc++/abi/unwind] Rename Lit features for no exceptions to -fno-exceptions.

no-exceptions sounds a bit better to me as a feature name rather than the Clang command-line argument, although what you have is probably okay too. (Unifying sounds great! I'll let someone else sign off though.)

Apr 17 2020, 4:59 PM · Restricted Project, Restricted Project, Restricted Project
dexonsmith committed rGb8d08e961df1: ADT: SmallVector size/capacity use word-size integers when elements are small (authored by browneee).
ADT: SmallVector size/capacity use word-size integers when elements are small
Apr 17 2020, 4:34 PM
dexonsmith closed D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

GIT_COMMITTER_NAME=Andrew Browne
GIT_COMMITTER_EMAIL=browneee@google.com

This would be my second commit. I will request access next time - thanks @dexonsmith!

Apr 17 2020, 4:34 PM · Restricted Project, Restricted Project

Apr 16 2020

dexonsmith added a comment to D74651: Add IR constructs for inalloca replacement preallocated call setup.
In D74651#1987364, @rnk wrote:

Does the LangRef patch typically come before, after, or in the same commit? After doing some experimenting?

I guess in this case it would be best to put together a LangRef patch to land before this one, in case anyone wants to look at it alone, without the boilerplate.

Apr 16 2020, 1:23 PM · Restricted Project

Apr 15 2020

dexonsmith added a comment to D74813: Function block naming - add hash of parameter type to end of block name.

Yes, I like the look of that. Can you also update the demangler to reverse this nicely? @erik.pilkington can help there. There are two copies. Usually you modify the copy in libcxxabi/ and then run a script to copy the changes into llvm/.

Apr 15 2020, 10:24 AM · Restricted Project, Restricted Project

Apr 14 2020

dexonsmith added a comment to D74813: Function block naming - add hash of parameter type to end of block name.

@erik.pilkington / @vsk / @dexonsmith - how block name = hash(parent_function_name + block_type) ?
So any blocks that are inside the same parent function + have the same type will get an incremental number to de-duplicate names.

Apr 14 2020, 7:03 PM · Restricted Project, Restricted Project
dexonsmith requested changes to D77830: [RFC] Generate more constant iVar Offsets via global LTO analysis .

I'll look into adding what needs to be added to have this also work for non-LTO. I was imagining It would be lot more integration.
Any other thoughts on this ?

Apr 14 2020, 12:58 PM · Restricted Project

Apr 13 2020

dexonsmith added a comment to D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

@dexonsmith I am not a committer, if the last changes looks good please submit for me. Thanks!

Apr 13 2020, 8:38 PM · Restricted Project, Restricted Project
dexonsmith accepted D77994: Move ModuleSummaryAnalysis from libAnalysis to libObject to break the dependency from Analysis to Object.

LGTM. Seems like a reasonable new home.

Apr 13 2020, 3:53 PM · Restricted Project
dexonsmith accepted D77772: [Clang] Expose RequiresNullTerminator in FileManager..

LGTM.

Apr 13 2020, 3:15 PM · Restricted Project
dexonsmith added a comment to D70284: Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping.

For some reason this revision is locked down and I'm not allowed to "edit" it, which includes adding inline review comments. Can you add me as a reviewer?

Thought I did.

Apr 13 2020, 2:07 PM · Restricted Project
dexonsmith accepted D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

Thanks for your patience, I missed the updates on Friday.

Apr 13 2020, 1:36 PM · Restricted Project, Restricted Project

Apr 9 2020

dexonsmith added a comment to D77772: [Clang] Expose RequiresNullTerminator in FileManager..

Not really. It's a static function in MemoryBuffer.cpp, and the MemoryBuffer class doesn't have a Kind member so we can't check for MemoryBufferMMapFile.

Apr 9 2020, 4:35 PM · Restricted Project
dexonsmith added a comment to D77772: [Clang] Expose RequiresNullTerminator in FileManager..

Code change LGTM.

Apr 9 2020, 3:48 PM · Restricted Project

Apr 8 2020

dexonsmith added a comment to D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute.

Maybe we can add an additional string attribute when adding the noinline attribute to functions which are not marked noinline in the source code, something like "noinline-added-by-clang". I don't know if that's a legitimate use case for a string attribute, but it wouldn't be very invasive. What do you think?

Another option (not sure if it's better) would be to add a noopt LLVM attribute that Clang adds for -O0 instead of noinline. Two possibilities would be to update the inliner to pay attention to that as well (with special logic for flatten), or to change the always-inliner to add noinline to anything marked noopt.

noopt == optnone? Both optnone and noinline are set in O0, so we could just not place noinline (I think).

Apr 8 2020, 10:07 PM · Restricted Project, Restricted Project
dexonsmith updated subscribers of D77697: libc++: adjust modulemap for non-modular C.

@dexonsmith - yeah, sadly I dont think that there is a good way to audit that - any change to the public headers can cause issues. Furthermore, the libc headers themselves also influence this.

Apr 8 2020, 4:18 PM · Restricted Project
dexonsmith added a comment to D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute.

Maybe we can add an additional string attribute when adding the noinline attribute to functions which are not marked noinline in the source code, something like "noinline-added-by-clang". I don't know if that's a legitimate use case for a string attribute, but it wouldn't be very invasive. What do you think?

Another option (not sure if it's better) would be to add a noopt LLVM attribute that Clang adds for -O0 instead of noinline. Two possibilities would be to update the inliner to pay attention to that as well (with special logic for flatten), or to change the always-inliner to add noinline to anything marked noopt.

Apr 8 2020, 3:46 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute.

Maybe we can add an additional string attribute when adding the noinline attribute to functions which are not marked noinline in the source code, something like "noinline-added-by-clang". I don't know if that's a legitimate use case for a string attribute, but it wouldn't be very invasive. What do you think?

Apr 8 2020, 3:46 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute.

TBH, I would issue a warning if we see flatten in O0 that says this will not work and be done with it.

Apr 8 2020, 3:13 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

Do we want to increase the complexity of SmallVector somewhat? or do we want to keep the limit and affirm SmallVector is for small things?

Apr 8 2020, 11:24 AM · Restricted Project, Restricted Project

Apr 7 2020

dexonsmith added a comment to D77697: libc++: adjust modulemap for non-modular C.

This SGTM in principle, but the patch makes me wonder: which other headers from the non-modularized C runtime change ownership?

Apr 7 2020, 6:32 PM · Restricted Project
dexonsmith requested changes to D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

Requesting changes just to be sure we consider the other options. I don't think it's good that SmallVector is no longer useful for large byte streams; I would prefer to fix that then stop using the type.

Apr 7 2020, 6:32 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D77621: ADT: SmallVector size/capacity use word-size integers when elements are small.

This is thanks to a commit of mine that shaved a word off of SmallVector. Some options to consider:

Apr 7 2020, 6:32 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D77586: Allow parameter names to be elided in a function definition in C.

This also adds the same feature to ObjC blocks under the assumption that ObjC wishes to follow the C standard in this regard.

SGTM - thats generally been the idea for blocks.

Apr 7 2020, 11:55 AM

Apr 2 2020

dexonsmith committed rG0c85c488e2b5: utils: Tweak clang-parse-diagnostics-file for modules includes (authored by dexonsmith).
utils: Tweak clang-parse-diagnostics-file for modules includes
Apr 2 2020, 2:39 PM
dexonsmith closed D77321: utils: Tweak clang-parse-diagnostics-file for modules includes.

LGTM.

Apr 2 2020, 2:38 PM
dexonsmith resigned from D76594: [clang][AST] Support AST files larger than 512M.

@rsmith, @dexonsmith, @jdoerfert could you please take a look to this diff?
If you think that there are other reviewers who might have more context AST persistence, please add them.

Apr 2 2020, 1:33 PM · Restricted Project
dexonsmith created D77321: utils: Tweak clang-parse-diagnostics-file for modules includes.
Apr 2 2020, 10:50 AM

Mar 31 2020

dexonsmith added a comment to D77058: [Clang] Add llvm.loop.unroll.disable to loops with -fno-unroll-loops..

I think this is a good approach, rather than a per-function attribute, since as mentioned this will be preserved through inlining.
@dexonsmith, does that seem reasonable to you? I missed the original patch and agree with you that we don't want to fix this in LTO by passing the option through to LTO.

Mar 31 2020, 1:44 PM · Restricted Project

Mar 27 2020

dexonsmith updated subscribers of D76916: [Darwin] Respect -fno-unroll-loops during LTO..

@fhahn, please revert, this isn't how we usually pass options in LTO.

Mar 27 2020, 6:12 PM · Restricted Project

Mar 18 2020

dexonsmith added a comment to D76290: [lit] Allow passing extra commands to executeShTest.
In D76290#1930134, @yln wrote:

I don't understand yet what this enables. Please extend your commit message to elaborate a bit more for people (like me) who aren't familiar with libcxx testing.
Does this enable deriving new tests from existing source files or running existing tests (.{pass,fail,sh}.cpp) in a different way without needing to modify those dozens/hundreds of files?

The change itself looks innocent enough and I don't want to block you on adding a test if there are currently none at all. LGTM.

Mar 18 2020, 5:55 PM · Restricted Project

Mar 14 2020

dexonsmith added a comment to D76178: [lit] Recursively apply substitutions.

It would also be good to confirm that this isn't expensive in the normal case (one-level substitutions). Have you timed running the libc++ tests with and without this patch?

Mar 14 2020, 10:11 AM · Restricted Project
dexonsmith requested changes to D76178: [lit] Recursively apply substitutions.

This looks really cool. Can you add tests for this feature, somewhere inside llvm/utils/lit/tests?

Mar 14 2020, 10:11 AM · Restricted Project

Mar 13 2020

dexonsmith accepted D76075: Revert "[ObjC][ARC] Check the basic block size before calling DominatorTree::dominate".
Mar 13 2020, 11:49 AM · Restricted Project

Mar 12 2020

dexonsmith added a reviewer for D76075: Revert "[ObjC][ARC] Check the basic block size before calling DominatorTree::dominate": fhahn.

This SGTM. Ideally we'd confirm the compile-time explosion is fixed before landing this though.

Mar 12 2020, 11:56 AM · Restricted Project

Mar 4 2020

dexonsmith added a comment to D72068: [IR] Refactor SubclassData.

Given the contention around direction, I would suggest that you seek consensus via an RFC on llvm-dev before pinging the patch any further.

It is the first thing that I did, but I was asked to post the patch for review. I agree that there should have been more discussion in the mail, but it seems stranger (and with not much response) to ping a mail thread.
Please refer to http://lists.llvm.org/pipermail/llvm-dev/2020-January/138710.html for further details.

Mar 4 2020, 4:20 PM · Restricted Project
dexonsmith added a comment to D72068: [IR] Refactor SubclassData.
In D72068#1889825, @rnk wrote:

I'm still basically not in favor of this. If another contributor thinks it's a great idea, I wouldn't go so far as to block this change, but I don't plan to do any further review.

Fair enough, I respect your opinion.
However, I still think it is a very welcoming change (for the many reasons listed in the "summary"), and I hope other reviewers will see the benefits as I see them, and approve this change.

Mar 4 2020, 11:47 AM · Restricted Project
dexonsmith added inline comments to D72860: [modules] Do not cache invalid state for modules that we attempted to load..
Mar 4 2020, 10:38 AM · Restricted Project

Mar 2 2020

dexonsmith added a comment to D75395: [clang][Modules] Add -fsystem-module flag.

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

I had another thought. What if for the explicitly built "system" modules:

  • If -Wsystem-headers is on, leave them as user modules in the explicit build.
  • If -Wsystem-headers is off, turn off all diagnostics in the explicit build.

Does that give the right semantics, or is there something subtly different?

I considered this, but decided against it because I wanted the implicit and explicit builds to be as similar as possible, and reduce the amount of changes made to the original command line. There's a lot of code in Clang dealing with system files, and I'm not 100% sure that -Wno-everything would be equivalent.

Mar 2 2020, 3:53 PM · Restricted Project
dexonsmith added a comment to D75395: [clang][Modules] Add -fsystem-module flag.

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

Mar 2 2020, 2:09 PM · Restricted Project
dexonsmith added inline comments to D75395: [clang][Modules] Add -fsystem-module flag.
Mar 2 2020, 2:09 PM · Restricted Project

Feb 28 2020

dexonsmith added a reviewer for D75323: Support relative dest paths in headermap files: bruno.
Feb 28 2020, 12:01 PM · Restricted Project
dexonsmith accepted D75213: RFC: More principled implementation of DISubprogram::describes().

Okay, LGTM then, as long as...

Feb 28 2020, 11:51 AM · Restricted Project, debug-info
dexonsmith added a comment to D75213: RFC: More principled implementation of DISubprogram::describes().

I'm inclined to try to commit this and see if we get any "describes" assertion reports.

Feb 28 2020, 9:55 AM · Restricted Project, debug-info

Feb 26 2020

dexonsmith added a comment to D75213: RFC: More principled implementation of DISubprogram::describes().

If you're worried about LTO, I suggest trying a clang bootstrap with each of -flto=full and -flto=thin. I was using the former as a testcase when I was doing this work, so I think we'd get good signal from that.

Feb 26 2020, 3:15 PM · Restricted Project, debug-info