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 (379 w, 2 d)

Recent Activity

Today

dexonsmith added a comment to D104819: [ADT] Rename StringRef case insensitive methods for clarity.

Just be sure to clang-format when you do the mechanical changes in the follow up patches.)

Hmm, those would have to be manually reviewed, but I guess I can try to do that (discarding changes where the surrounding code isn't too well formatted now already so it does more extensive reformattings other than for the renaming).

Thu, Jun 24, 12:27 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
dexonsmith accepted D103503: [OpaquePtr] Introduce option to force all pointers to be opaque pointers.

This LGTM, once you add the RUN line for llvm-dis I suggested in the last comment. It's up to you whether to split out the changes for moving the LLVMContext construction after command-line parsing into a prep commit... there could be an argument for keeping it together as well.

Thu, Jun 24, 12:23 PM · Restricted Project
dexonsmith accepted D104819: [ADT] Rename StringRef case insensitive methods for clarity.

LGTM, once @MaskRay is happy. I have a couple of minor comments inline too.

Thu, Jun 24, 11:54 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
dexonsmith added a comment to D103503: [OpaquePtr] Introduce option to force all pointers to be opaque pointers.

(Apologies for disappearing; I was out for a bit, and then lost track of this review.)

Thu, Jun 24, 11:43 AM · Restricted Project
dexonsmith updated subscribers of D104516: [mlir] Add a ThreadPool to MLIRContext and refactor MLIR threading usage.

The MLIR bot, https://lab.llvm.org/buildbot/#/builders/88/builds/14468, Have been red for over 30hr when this patch landed. Please provide a fix or pull the related patches to bring the bot back to green.

Are those related to this patch? Those all look like orc failures.

I think pulling in additional code has caused a relocation overflow in the runtime linker. We might have to switch to the large code model for ORC on PPC.

Thanks for looking! That definitely looks like what is happening. Do you know how to adjust that? (I have very little experience/knowledge of ORC)

Thu, Jun 24, 11:11 AM · Restricted Project, Restricted Project

Yesterday

dexonsmith accepted D104820: [ADT] Complete the StringRef case insensitive method renaming.

LGTM, once all the code is migrated.

Wed, Jun 23, 4:19 PM · Restricted Project
dexonsmith added a comment to D104819: [ADT] Rename StringRef case insensitive methods for clarity.

Given how large this is, would it be reasonable to split this up a bit more?

Wed, Jun 23, 4:18 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
dexonsmith committed rG3cf415c6c367: IR: Fix use-list-order round-tripping for call and invoke (authored by dexonsmith).
IR: Fix use-list-order round-tripping for call and invoke
Wed, Jun 23, 12:06 PM
dexonsmith closed D104805: IR: Fix use-list-order round-tripping for call and invoke.
Wed, Jun 23, 12:06 PM · Restricted Project
dexonsmith added a comment to D104805: IR: Fix use-list-order round-tripping for call and invoke.

Can you please point me to the code where the use-list order prediction is made?

I'll need a minute to find it for BitcodeWriter.

Wed, Jun 23, 11:52 AM · Restricted Project
dexonsmith added a comment to D104805: IR: Fix use-list-order round-tripping for call and invoke.

Can you please point me to the code where the use-list order prediction is made?

Wed, Jun 23, 11:48 AM · Restricted Project
dexonsmith updated the diff for D104805: IR: Fix use-list-order round-tripping for call and invoke.

Minor update to the testcase:

Wed, Jun 23, 11:44 AM · Restricted Project
dexonsmith added a comment to D104740: [OpaquePtr] Support call instruction.

I'll look for other similar bugs and post a patch.

Wed, Jun 23, 11:33 AM · Restricted Project
dexonsmith requested review of D104805: IR: Fix use-list-order round-tripping for call and invoke.
Wed, Jun 23, 11:31 AM · Restricted Project
dexonsmith added a comment to D104740: [OpaquePtr] Support call instruction.

Preserve address space check, add test with argument.

While doing the latter, I found out that

define void @call(void (...)* %p) {
  call void (...) %p(void (...)* %p)
  ret void
}

fails verify-uselistorder (independently of this patch). I happened to use that as the first thing I tried :/

Interesting; given that there are two uses of %p maybe one of them isn't being tracked. Did you investigate?

Wed, Jun 23, 10:52 AM · Restricted Project
dexonsmith added a comment to D104740: [OpaquePtr] Support call instruction.

Preserve address space check, add test with argument.

While doing the latter, I found out that

define void @call(void (...)* %p) {
  call void (...) %p(void (...)* %p)
  ret void
}

fails verify-uselistorder (independently of this patch). I happened to use that as the first thing I tried :/

Wed, Jun 23, 10:22 AM · Restricted Project
dexonsmith added a comment to D104218: [ADT] Add StringRef consume_front_lower and consume_back_lower.

That said, xxx_lower() is a really terrible name here for insensitive comparisons. We have StringRef::lower() which returns a lower case string - it isn't insensitive. It seems that the whole family needs a rename. Could you take a crack at this as a follow-up? It should be a mechanical change.

Sure, I can give that a shot. Any suggestion on what the name pattern should be? xxx_caseinsensitive() is a bit of a mouthful, xxx_i() is terse but less understandable and discoverable.

A few ideas to consider:

  • X_casei()
  • X_nocase()
  • X_ignorecase()
  • X_foldcase() (term from text search)

Thanks for the suggestions! What do you think of @lattner ‘s suggestion (X_insensitive)?

Wed, Jun 23, 10:17 AM · Restricted Project

Tue, Jun 22

dexonsmith added a comment to D104663: [OpaquePtr] Remove checking pointee type for byval/preallocated type.

As for preallocated, my stance is still the same. It's not in used at all anywhere (I'm the person who implemented it but never got around to finishing it up).

Tue, Jun 22, 12:28 PM · Restricted Project
dexonsmith accepted D104484: [clang] Add cc1 option for dumping layout for all complete types.

LGTM.

Tue, Jun 22, 11:31 AM · Restricted Project
dexonsmith added a comment to D57688: [libcxx] Remove <ext/hash_set>, <ext/hash_map> and <ext/__hash>.

I might revisit this later, but I'm not moving forward for the time being since a bunch of people said it was going to break them.

I think we should instead ship those headers as a separate project and people who need them can still get the headers from that project. IOW, they shouldn't be part of libcxx. But I don't have any time to work on this low priority cleanup at the moment, so abandoning.

Tue, Jun 22, 11:27 AM
dexonsmith added a comment to D104218: [ADT] Add StringRef consume_front_lower and consume_back_lower.

That said, xxx_lower() is a really terrible name here for insensitive comparisons. We have StringRef::lower() which returns a lower case string - it isn't insensitive. It seems that the whole family needs a rename. Could you take a crack at this as a follow-up? It should be a mechanical change.

Sure, I can give that a shot. Any suggestion on what the name pattern should be? xxx_caseinsensitive() is a bit of a mouthful, xxx_i() is terse but less understandable and discoverable.

Tue, Jun 22, 10:54 AM · Restricted Project

Mon, Jun 21

dexonsmith added a comment to D104663: [OpaquePtr] Remove checking pointee type for byval/preallocated type.

I agree with @dblaikie that it'd be nice to get a clear answer for the behaviour (and documenting it in LangRef). Also, I wonder about ignoring the instruction attribute when the function declaration exists... seems like that is a step toward removing the instruction attribute entirely (maybe that's the right thing to do (not sure it is), but if so then it should be a high-level bit, not a side effect).

Mon, Jun 21, 4:23 PM · Restricted Project

Fri, Jun 18

dexonsmith closed D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12.

I pushed this as 05d0f1a8ea012a6b7b8ea65893ec4121106444b5 last night, but I just realized I forgot to include the link to the review :(. Closing manually.

Fri, Jun 18, 10:59 AM · Restricted Project

Thu, Jun 17

dexonsmith committed rG05d0f1a8ea01: Frontend: Respect -fno-temp-file when creating a PCH (authored by zahen).
Frontend: Respect -fno-temp-file when creating a PCH
Thu, Jun 17, 6:34 PM
dexonsmith added a comment to D104465: [clang][deps] Prevent PCH validation failures by padding minimized files.

A few other ideas:

  1. Disable PCH validation when minimizing. (Is that less robust than your current workaround? If so, can you explain why?)
  2. Use the original PCH header in the scanning -cc1s (translate -include-pch to -include) and switch back in the generated -cc1s (back to -include-pch).
  3. Embed instructions in the PCH for how to build it, and make a "minimized" version of the PCH.
Thu, Jun 17, 3:15 PM · Restricted Project
dexonsmith added a comment to D104459: [clang][lex] Ensure minimizer output is never larger than input.

Another thing to consider: a client that wants the minimized source to be "no bigger than" the original source can use the original source if it ends up growing. For example, https://reviews.llvm.org/D104462 could check the resulting size, and just return the original source in the (extremely unlikely?) corner case where the minimized sources are bigger.

Thu, Jun 17, 2:13 PM · Restricted Project
dexonsmith added inline comments to D104484: [clang] Add cc1 option for dumping layout for all complete types.
Thu, Jun 17, 1:28 PM · Restricted Project
dexonsmith added inline comments to D103930: [clang][HeaderSearch] Fix implicit module when using header maps.
Thu, Jun 17, 12:42 PM · Restricted Project
dexonsmith accepted D104350: [clang][AST] Make `getLocalOrImportedSubmoduleID` work with const `Module*`. NFC..

LGTM.

Thu, Jun 17, 12:17 PM · Restricted Project
dexonsmith requested changes to D104462: [clang][lex] Add minimizer option to pad the output to the input size.

I'm not sure D104465 (the motivation for this patch) is the right approach, but we can have that discussion over there.

Thu, Jun 17, 11:57 AM · Restricted Project
dexonsmith added a comment to D104465: [clang][deps] Prevent PCH validation failures by padding minimized files.

This patch fixes dependency scanning of a TU with prebuilt modular/PCH dependencies.

Such modules/PCH might have been built using non-minimized files, while dependency scanner does use the minimized versions of source files. Implicit build doesn't like the discrepancy in file sizes and errors out.

The issues is worked around by padding the minimized files with whitespace in such scenarios. This approach throws away one advantage of source minimization (the memory savings), but still keeps the benefits of faster preprocessing/lexing.

Thu, Jun 17, 11:43 AM · Restricted Project
dexonsmith added a comment to D104459: [clang][lex] Ensure minimizer output is never larger than input.

This patch ensures the output of source minimizer is never larger than the input. This property will be handy in a follow up patch that makes it possible to pad the minimized output to the size of the original input.

Thu, Jun 17, 11:18 AM · Restricted Project
dexonsmith updated subscribers of D103930: [clang][HeaderSearch] Fix implicit module when using header maps.

Thanks for working on this, comments inline. @vsapsai @jansvoboda11 @dexonsmith any headermap related concerns on your side?

Thu, Jun 17, 10:53 AM · Restricted Project

Tue, Jun 15

dexonsmith committed rG3302af9d4c39: Support: Remove F_{None,Text,Append} compatibility synonyms, NFC (authored by dexonsmith).
Support: Remove F_{None,Text,Append} compatibility synonyms, NFC
Tue, Jun 15, 12:05 PM
dexonsmith closed D101506: Support: Remove F_{None,Text,Append} compatibility synonyms, NFC.
Tue, Jun 15, 12:04 PM · Restricted Project, Restricted Project, Restricted Project

Sun, Jun 13

dexonsmith added a comment to D103807: [clang][deps] Ensure deterministic order of TU '-fmodule-file=' arguments.

I'm not sure the performance problems with std::map will matter in practice here, but have you considered sorting before emission rather than relying on the data structure's iteration order? (It'd make it easy to switch to StringMap in the future.)

Sun, Jun 13, 1:24 PM · Restricted Project
dexonsmith accepted D103526: [clang][deps] Handle modular dependencies present in PCH.

LGTM.

Sun, Jun 13, 1:09 PM · Restricted Project
dexonsmith accepted D103802: [clang][modules][pch] Allow loading PCH with different modules cache path.

LGTM.

Sun, Jun 13, 1:08 PM · Restricted Project
dexonsmith accepted D104012: [clang][deps] Move stripping of diagnostic serialization from `clang-scan-deps` to `DependencyScanning` library.

LGTM too.

Sun, Jun 13, 1:06 PM · Restricted Project
dexonsmith accepted D103461: [clang][deps] NFC: Preserve the original frontend action.

Okay, LGTM. (Sorry for the delay, I've been out.)

Sun, Jun 13, 1:05 PM · Restricted Project
dexonsmith accepted D104036: [clang][deps] Prevent unintended modifications of the original TU command-line.

LGTM, with one suggestion inline.

Sun, Jun 13, 1:02 PM · Restricted Project
dexonsmith accepted D104104: [clang][deps] NFC: Handle `DependencyOutputOptions` only once.

LGTM.

Sun, Jun 13, 12:58 PM · Restricted Project
dexonsmith accepted D104106: [clang][deps] NFC: Stop using moved-from object.

LGTM.

Sun, Jun 13, 12:57 PM · Restricted Project
dexonsmith accepted D102763: [LLParser] Remove outdated deplibs.

Given the thread on the mailing list, I think that this is fine as long as there is an accompanying test to validate the bitcode deserialization continues to work. Id wait a few days more to see if @dexonsmith has an opinion.

Sun, Jun 13, 12:55 PM · Restricted Project, Restricted Project

Thu, Jun 3

dexonsmith added reviewers for D102943: Hashing: use a 64-bit storage type on all platforms.: jansvoboda11, Bigcheese.

This looks much cleaner; thanks for the update!

Thu, Jun 3, 2:05 PM · Restricted Project, Restricted Project, Restricted Project

Wed, Jun 2

dexonsmith added inline comments to D103503: [OpaquePtr] Introduce option to force all pointers to be opaque pointers.
Wed, Jun 2, 11:55 AM · Restricted Project

Tue, Jun 1

dexonsmith added a comment to D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12.

@zahen , what author information should I use for the Git commit?

Tue, Jun 1, 12:53 PM · Restricted Project
dexonsmith accepted D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12.

LGTM; thanks for you patience.

Tue, Jun 1, 12:52 PM · Restricted Project
dexonsmith added a comment to D103461: [clang][deps] NFC: Preserve the original frontend action.

Is there (or could there be) a mode where clang-scan-deps prints out the command-lines it sends to the dependency scanning action (maybe instead of actually scanning), so this could be tested?

Tue, Jun 1, 12:12 PM · Restricted Project
dexonsmith added inline comments to D102923: [clang][lex] Remark for used header search paths.
Tue, Jun 1, 12:05 PM · Restricted Project

Fri, May 28

dexonsmith added a comment to D103165: Threading: use independent llvm::thread implementation on Apple platforms to increase stack size.

I guess one option would be to never use std::thread, and repurpose the guts of llvm_execute_on_thread_impl to implement a cross-platform llvm::thread that allows specifying the stack size as an optional constructor parameter (and then change llvm_execute_on_thread to use llvm::thread). WDYT?

The llvm_execute_on_thread family only seems to have three users and is definitely a less appealing interface, I'd be very tempted round this llvm::thread out as you suggest and switch all users to it.

Fri, May 28, 2:19 PM · Restricted Project, Restricted Project
dexonsmith requested changes to D102923: [clang][lex] Remark for used header search paths.

Thanks for your patience! I thought I already looked at this last week.

Fri, May 28, 2:10 PM · Restricted Project
dexonsmith requested changes to D103275: Update musttail verification to check all swiftasync->swiftasync tail calls..

I guess that the 'in tail position' restriction cannot really be used by frontends because it has to predict that otherStuff() cannot be optimized away. So it would not buy us much other than a loss of clarity ...

Fri, May 28, 12:03 PM · Restricted Project
dexonsmith added reviewers for D103275: Update musttail verification to check all swiftasync->swiftasync tail calls.: dexonsmith, t.p.northover.
Fri, May 28, 11:15 AM · Restricted Project

Thu, May 27

dexonsmith added a comment to D102943: Hashing: use a 64-bit storage type on all platforms..

This new version is an attempt to have modules not rely on llvm::hash_code, but on a new llvm::stable_hash_code.
I understand modifying ADT/Hashing.h is sensitive, so maybe we need to discuss the high-level approach first.

Thu, May 27, 5:44 PM · Restricted Project, Restricted Project, Restricted Project
dexonsmith updated subscribers of D103275: Update musttail verification to check all swiftasync->swiftasync tail calls..

I guess I'm wondering what the trajectory is - if the goal is for this to eventually be an IR invariant for the calling convention, I think a verifier check makes sense; once the codegen bugs are sorted out, and the bitcode upgrade is done, the flag can be switched to on-by-default and eventually removed. But if this will never be an invariant of the IR, I'm not sure the LLVM IR verifier is really the right place for the check.

Thu, May 27, 1:48 PM · Restricted Project
dexonsmith added inline comments to D103275: Update musttail verification to check all swiftasync->swiftasync tail calls..
Thu, May 27, 1:23 PM · Restricted Project
dexonsmith added a comment to D103275: Update musttail verification to check all swiftasync->swiftasync tail calls..

I used Assert for two reasons: (1) the surrounding code uses Assert, so it seemed like the right thing to use and (2) in case there's a failure, it will display the instruction missing the musttail.

Thu, May 27, 1:21 PM · Restricted Project
dexonsmith added inline comments to D103275: Update musttail verification to check all swiftasync->swiftasync tail calls..
Thu, May 27, 1:14 PM · Restricted Project
dexonsmith added a comment to D103275: Update musttail verification to check all swiftasync->swiftasync tail calls..

This doesn't look like a particularly expensive verifier check (although maybe I've missed something). What's the motivation for hiding it behind EnableSwiftTailCCMustTailCheck? Have you considered using bitcode AutoUpgrade to fix old IR?

Thu, May 27, 1:02 PM · Restricted Project
dexonsmith added inline comments to D103275: Update musttail verification to check all swiftasync->swiftasync tail calls..
Thu, May 27, 12:52 PM · Restricted Project
dexonsmith accepted D103229: [clang] NFC: split HeaderMapTest to have re-usable header map implementation for testing.

LGTM; thanks for splitting this out.

Thu, May 27, 9:00 AM · Restricted Project

Wed, May 26

dexonsmith added a comment to D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics.

Naively, this sounds like it could be a non-trivial tax on build times. But it looks like it's only called in Clang from Sema::diagnoseMissingImport, which only happens on error anyway.

Wed, May 26, 5:52 PM · Restricted Project
dexonsmith updated subscribers of D103165: Threading: use independent llvm::thread implementation on Apple platforms to increase stack size.

@dblaikie, wouldn't mind your thoughts on this idea (expanding the interface of llvm::thread to include an optional stack size parameter).

Wed, May 26, 9:13 AM · Restricted Project, Restricted Project
dexonsmith added a comment to D103165: Threading: use independent llvm::thread implementation on Apple platforms to increase stack size.

I wonder, could this be used to simplify RunSafelyOnThread, and/or llvm_execute_on_thread_impl? I figure all that complexity is because of the same limitation, but maybe it's not something we can fix, since we exposed a libclang API (clang_executeOnThread) that allows specifying a stack size.

Wed, May 26, 9:09 AM · Restricted Project, Restricted Project

May 21 2021

dexonsmith updated subscribers of D102943: Hashing: use a 64-bit storage type on all platforms..

Isn't the bug here that module hashing is using hash_code? So shouldn't the correct fix be to use a specific hashing algorithm for module hashes?

May 21 2021, 6:20 PM · Restricted Project, Restricted Project, Restricted Project
dexonsmith updated subscribers of D102633: [clang-scan-deps] Improvements to thread usage.

It might be good for @aganea to take a look as well.

Thanks! I actually work with @saudi, I already took a look at the patch before uploading. However I'm stil torn about running one of the workers on the main thread. I fear that we could have random errors because of the stack size of the "main" thread could be different from the stack size of the "satellite" threads. There's 99.99% chance that this won't happen, but I'd prefer that behavior to be explicit.

We could have:

-j0: use all hardware threads
-j1: don't use multi-threading, run all the tasks on the main thread
-jN: use the specified number of threads

The rationale is that we're using clang-scan-deps as-a-DLL in Fastbuild, to extract the dependencies. Since Fastbuild already has its own thread pool management, we call into clang-scan-deps with -j1 from different Fastbuild threads (but keeping the DependencyScanningService alive between calls). It would great if each call to clang-scan-deps wouldn't create a extra new thread.

Perhaps any of you would like to comment? +@mehdi_amini

May 21 2021, 6:10 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D102726: [IR][AutoUpgrade] Drop alignment from non-pointer parameters and returns.

FYI, this change causes a non-trivial compile-time regression in LTO builds: https://llvm-compile-time-tracker.com/compare.php?from=136ced498ba84f6b6126051626e319f18ba740f5&to=5b6cae5524905bc43cfc21a515f828528d1f2e68&stat=instructions (Worst regression seems to be the ThinLTO kc.link step, which is up 4%.)

LLVM attributes are optimized for immutable usage, and it's hard to overstate just how slow the mutable APIs on them are...

I suggested not to check for attributes just to remove them. I still think that's the right thing *but* we can check for them in the remove functions.
@nikic you think that will resolve the compile time issue?

May 21 2021, 5:49 PM · Restricted Project
dexonsmith added a comment to D102943: Hashing: use a 64-bit storage type on all platforms..

why do module hashes need to be stable when cross-compiling?

May 21 2021, 5:38 PM · Restricted Project, Restricted Project, Restricted Project
dexonsmith accepted D102924: [clang] NFC: Replace std::pair by a struct in InitHeaderSearch.

LGTM.

May 21 2021, 4:36 PM · Restricted Project
dexonsmith accepted D102880: [llvm] Revert align attr test in test/Bitcode/attribute-3.3.ll.

LGTM! I suggest mentioning the original commit (0a8e12fdbbf54e81954b091ac9fb11cd5f5148e1) and the commit that changed the bitcode (2a078c3072043541ee0595aea6c8d7909f94c6f9) in your commit message.

May 21 2021, 12:58 PM · Restricted Project

May 20 2021

dexonsmith added a comment to D102726: [IR][AutoUpgrade] Drop alignment from non-pointer parameters and returns.

I will do that as a followup.

May 20 2021, 12:57 PM · Restricted Project
dexonsmith added a comment to D102726: [IR][AutoUpgrade] Drop alignment from non-pointer parameters and returns.

Can you include the revert to attributes-3.3.ll.bc as part of this?

I don't think that matters too much

May 20 2021, 12:47 PM · Restricted Project

May 19 2021

dexonsmith added a comment to D102726: [IR][AutoUpgrade] Drop alignment from non-pointer parameters and returns.

Can you include the revert to attributes-3.3.ll.bc as part of this?

May 19 2021, 3:03 PM · Restricted Project

May 17 2021

dexonsmith added inline comments to D87304: [AttributeFuncs] Consider `align` in `typeIncompatible`.
May 17 2021, 11:46 AM · Restricted Project

May 15 2021

dexonsmith added a comment to D102201: [IR][AutoUpgrade] Drop align attribute from void return types.

We bisected a +153kB binary size increase for chrome/android to this (https://bugs.chromium.org/p/chromium/issues/detail?id=1208513#c4). Is that expected? (That build config uses -Oz for many TUs.)

May 15 2021, 6:39 AM · Restricted Project
dexonsmith updated subscribers of D87304: [AttributeFuncs] Consider `align` in `typeIncompatible`.
May 15 2021, 6:35 AM · Restricted Project

May 14 2021

dexonsmith added a comment to D102523: [docs] Explain address spaces a bit more in opaque pointers doc.

The new paragraph or something similar LGTM, although probably @theraven should be the one to sign off given the post-commit feedback.

May 14 2021, 3:34 PM · Restricted Project
dexonsmith accepted D102495: [clang][deps] Support inferred modules.

LGTM, thanks for splitting out and moving the tool to clang/utils.

May 14 2021, 12:47 PM · Restricted Project
dexonsmith accepted D102491: [clang][modules] Build inferred modules.

Looks great — thanks for splitting this out!

May 14 2021, 11:57 AM · Restricted Project
dexonsmith requested changes to D102488: [clang][deps] Prune unused header search paths.

The code looks mostly good; some inline comments.

May 14 2021, 11:54 AM · Restricted Project
dexonsmith accepted D102482: [clang][deps] NFC: Report modules' context hash.

LGTM, although I'd slightly prefer the change to CompilerInstance.h be split out and committed after.

May 14 2021, 11:27 AM · Restricted Project
dexonsmith accepted D102473: [clang][deps] NFC: Stop assuming the TU's context hash.

LGTM.

May 14 2021, 11:22 AM · Restricted Project
dexonsmith added a comment to D102292: [docs] Add page on opaque pointer types.

Yeah, that seems out of scope for this document. https://llvm.org/docs/LangRef.html#pointer-type documents that pointers to functions and pointers to other things are all roughly the same sort of thing - and we're changing all of them over to the opaque pointer type. & other wording generally treats all pointers as having the same size, etc. I think that's best left elsewhere than this doc if there's no interesting distinction between function pointers and data pointers currently (which it doesn't seem like there is).

Note that we have support for different code and data representations with the P flag in the DataLayout. We can also differentiate between pointers to globals with G and there's an under-review patch for a specified address space for allocas. I think it's worth highlighting in this document that address spaces are the recommended way of representing different kinds of pointers (with potentially different sizes, access to different types of memory, representations of null, and so on). We retain the distinction between pointer types where the semantics matter for lowering, we lose the distinction when the distinction doesn't convey any useful meaning.

This is good information to know (I didn't know about it before), but IMO address spaces should have their own documentation (that I'd be happy to link to here). This seems orthogonal to opaque pointers.

May 14 2021, 11:17 AM · Restricted Project

May 13 2021

dexonsmith accepted D102292: [docs] Add page on opaque pointer types.

LGTM too. This is great to have; thanks for writing this up!

May 13 2021, 2:27 PM · Restricted Project
dexonsmith committed rG7c57a9bd7d4c: Modules: Simplify how DisableGeneratingGlobalModuleIndex is set, likely NFC (authored by dexonsmith).
Modules: Simplify how DisableGeneratingGlobalModuleIndex is set, likely NFC
May 13 2021, 10:40 AM
dexonsmith closed D101672: Modules: Simplify how DisableGeneratingGlobalModuleIndex is set, likely NFC.
May 13 2021, 10:40 AM · Restricted Project
dexonsmith committed rG23e9146fba29: Modules: Rename ModuleBuildFailed => DisableGeneratingGlobalModuleIndex, NFC (authored by dexonsmith).
Modules: Rename ModuleBuildFailed => DisableGeneratingGlobalModuleIndex, NFC
May 13 2021, 10:23 AM
dexonsmith closed D101670: Modules: Rename ModuleBuildFailed => DisableGeneratingGlobalModuleIndex, NFC.
May 13 2021, 10:22 AM · Restricted Project
dexonsmith committed rG7c2afd5899df: Modules: Remove ModuleLoader::OtherUncachedFailure, NFC (authored by dexonsmith).
Modules: Remove ModuleLoader::OtherUncachedFailure, NFC
May 13 2021, 10:11 AM
dexonsmith closed D101667: Modules: Remove ModuleLoader::OtherUncachedFailure, NFC.
May 13 2021, 10:11 AM · Restricted Project

May 11 2021

dexonsmith accepted D101704: [IR] Introduce the opaque pointer type.

update test
add to ReleaseNotes
mention that opaque pointer types aren't ready for general use yet

May 11 2021, 12:57 PM · Restricted Project
dexonsmith added inline comments to D102261: Introduce SYCL 2020 mode.
May 11 2021, 12:15 PM · Restricted Project

May 10 2021

dexonsmith added reviewers for D101704: [IR] Introduce the opaque pointer type: t.p.northover, dexonsmith.

The code LGTM too, just a couple of nits on docs and tests. Not sure if @t.p.northover has time to look right now, but I added him in case he does. I'm excited to see this moving along!

May 10 2021, 6:09 PM · Restricted Project
dexonsmith added a comment to D101667: Modules: Remove ModuleLoader::OtherUncachedFailure, NFC.

Ping! Happy to weaken the commit message to "likely NFC" in case there's some way that we get back here after a fatal error.

May 10 2021, 3:47 PM · Restricted Project
dexonsmith accepted D102201: [IR][AutoUpgrade] Drop align attribute from void return types.

LGTM with a couple of nits about the test.

May 10 2021, 3:35 PM · Restricted Project

May 5 2021

dexonsmith added inline comments to D100934: [clang][modules] Build inferred modules.
May 5 2021, 7:14 AM · Restricted Project
dexonsmith added inline comments to D100934: [clang][modules] Build inferred modules.
May 5 2021, 6:51 AM · Restricted Project

May 4 2021

dexonsmith added a comment to D100934: [clang][modules] Build inferred modules.

Given that there are four different things being done in this commit, it sounds naively like it should be four separate commits. If the logic is too intertwined to land each of the four pieces separately (certainly possible), can you quickly explain why, to motivate landing them together? (Or maybe there's an NFC refactoring that could be separated as a prep, to make the functional changes more isolated / easy to reason about?)

I think that landing everything together provides more context for people going through git log in the future.

If you really think splitting this up would be better, how about this?

  1. Fix AsWritten for umbrella headers/directories.
  2. Add tests to clang/test/Modules.
May 4 2021, 1:38 PM · Restricted Project

May 3 2021

dexonsmith added a comment to D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12.

Yep, this fixes a regression (https://bugs.llvm.org/show_bug.cgi?id=50033) caused by ad7aaa475e5e632242b07380ec47d2f35d077209, where I somehow missed that modules and PCHs had different actual behaviour around -fno-temp-file despite sharing the same comments.

May 3 2021, 11:45 AM · Restricted Project