rsmith (Richard Smith - zygoloid)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 12 2012, 2:19 PM (306 w, 1 d)

Recent Activity

Yesterday

rsmith added a comment to D41039: Add support for attribute "trivial_abi".

Yes, please document this in itanium-cxx-abi. Thanks!

Fri, May 25, 1:32 PM
rsmith added a comment to D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets.

make __map_node_handle use a const_cast on the key instead of type-punning between pair<const K, V> and pair<K, V>. Add calls to std::launder in key() and before inserting a node handle.

Fri, May 25, 12:45 PM

Thu, May 24

rsmith added a comment to D47341: [Sema] Disable creating new delayed typos while correcting existing..

Rather than disabling typo correction in TransformTypos, it would be preferable to attempt to immediately correct them. (That should allow the variableX.getX() case to still work.)

Thu, May 24, 4:38 PM
rsmith accepted D47229: Make atomic non-member functions as nonnull.

Either the previous version of this patch or a version with a bool factored out seem OK to me.

Thu, May 24, 3:24 PM
rsmith committed rC333234: Improve diagonstic for braced-init-list as operand to ?: expression..
Improve diagonstic for braced-init-list as operand to ?: expression.
Thu, May 24, 3:07 PM
rsmith committed rL333234: Improve diagonstic for braced-init-list as operand to ?: expression..
Improve diagonstic for braced-init-list as operand to ?: expression.
Thu, May 24, 3:06 PM
rsmith committed rL333233: Switch a couple of users of LangOpts::GNUMode to the more appropriate LangOpts….
Switch a couple of users of LangOpts::GNUMode to the more appropriate LangOpts…
Thu, May 24, 2:56 PM
rsmith committed rC333233: Switch a couple of users of LangOpts::GNUMode to the more appropriate LangOpts….
Switch a couple of users of LangOpts::GNUMode to the more appropriate LangOpts…
Thu, May 24, 2:55 PM
rsmith committed rL333220: Improve diagnostics for config mismatches with -fmodule-file..
Improve diagnostics for config mismatches with -fmodule-file.
Thu, May 24, 1:08 PM
rsmith committed rC333220: Improve diagnostics for config mismatches with -fmodule-file..
Improve diagnostics for config mismatches with -fmodule-file.
Thu, May 24, 1:08 PM
rsmith added a comment to D47067: Update NRVO logic to support early return.

Some minor nits. I would also like to see a test for the unoptimized (-O0) IR generated by Clang for the case where there are two NRVO variables in the same function. Otherwise, this looks good to go!

Thu, May 24, 11:54 AM

Wed, May 23

rsmith added a comment to D47067: Update NRVO logic to support early return.

Thanks! This looks like exactly the right way to compute when to apply NRVO. It'd be good to track (in your commit message at least) that this addresses PR13067.

Wed, May 23, 6:06 PM
rsmith added inline comments to D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers.
Wed, May 23, 5:44 PM
rsmith committed rC333141: Use zeroinitializer for (trailing zero portion of) large array initializers.
Use zeroinitializer for (trailing zero portion of) large array initializers
Wed, May 23, 4:48 PM
rsmith committed rL333141: Use zeroinitializer for (trailing zero portion of) large array initializers.
Use zeroinitializer for (trailing zero portion of) large array initializers
Wed, May 23, 4:48 PM
rsmith added a comment to D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable..
  • Currently clang errors out when aligned operator new is selected but the OS's version is too old to support it. What's the reason we want to change this now to be a warning rather than an error?
Wed, May 23, 4:37 PM
rsmith committed rL333126: Rework __builtin_classify_type support to better match GCC and to not assert on.
Rework __builtin_classify_type support to better match GCC and to not assert on
Wed, May 23, 2:23 PM
rsmith committed rC333126: Rework __builtin_classify_type support to better match GCC and to not assert on.
Rework __builtin_classify_type support to better match GCC and to not assert on
Wed, May 23, 2:23 PM

Tue, May 22

rsmith updated subscribers of D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type.
Tue, May 22, 7:48 PM · Restricted Project
rsmith added a comment to D47159: [InstCombine] Format String optimizations.

Converting to printf("single string literal") will likely be a small performance pessimization (because the string literal must be scanned for embedded % characters). I think the most efficient form is instead likely to be a format string comprising *only* format specifiers; for example, given

Tue, May 22, 6:04 PM
rsmith committed rL333044: Use zeroinitializer for (trailing zero portion of) large array initializers.
Use zeroinitializer for (trailing zero portion of) large array initializers
Tue, May 22, 5:13 PM
rsmith committed rC333044: Use zeroinitializer for (trailing zero portion of) large array initializers.
Use zeroinitializer for (trailing zero portion of) large array initializers
Tue, May 22, 5:13 PM
rsmith closed D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably.
Tue, May 22, 5:13 PM
rsmith added inline comments to D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably.
Tue, May 22, 12:53 PM
rsmith updated the diff for D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably.
Tue, May 22, 12:53 PM

Mon, May 21

rsmith added a comment to D46919: [libclang] Deprecate CXPrintingPolicy_IncludeTagDefinition.

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

This test prompted me to keep the IncludeTagDefinition member in PrintingPolicy so that clang_PrintingPolicy_getProperty would return the previous value set by clang_PrintingPolicy_setProperty. Otherwise, the value doesn't have any effect. Is that self-consistency not worth worrying about? If so, I'll remove both.

Mon, May 21, 4:50 PM
rsmith added a comment to D46439: [Sema] Fix incorrect packed aligned structure layout.

This wouldn't be another layout/ABI breakage, would it?

Mon, May 21, 4:45 PM
rsmith added a comment to D46241: [CodeGen] Recognize more cases of zero initialization.

I've done some more investigation, and I now think this patch is merely working around deficiencies elsewhere. See https://reviews.llvm.org/D47166, which aims to fix those deficiencies more directly.

Mon, May 21, 3:50 PM
rsmith created D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably.
Mon, May 21, 3:49 PM
rsmith added inline comments to D46241: [CodeGen] Recognize more cases of zero initialization.
Mon, May 21, 2:12 PM
rsmith committed rL332886: Revert r332847; it caused us to miscompile certain forms of reference….
Revert r332847; it caused us to miscompile certain forms of reference…
Mon, May 21, 1:41 PM
rsmith committed rC332886: Revert r332847; it caused us to miscompile certain forms of reference….
Revert r332847; it caused us to miscompile certain forms of reference…
Mon, May 21, 1:40 PM
rsmith committed rL332879: Revert r332028; see PR37545 for details..
Revert r332028; see PR37545 for details.
Mon, May 21, 1:17 PM
rsmith committed rC332879: Revert r332028; see PR37545 for details..
Revert r332028; see PR37545 for details.
Mon, May 21, 1:17 PM
rsmith added a comment to D46711: [private] Add min_vector_width function attribute. Use it to annotate all of the x86 intrinsic header files. Emit a attribute in IR.

I suspect you want at least one of Reid or Richard to look at this from the Clang side, but this is definitely the direction I was thinking.

Mon, May 21, 11:52 AM
rsmith added a comment to D47150: [Clang Tablegen] Add llvm_unreachable() to getModifierName().

Please instead add an llvm_unreachable outside the switch, so that we still get warnings on missing switch cases if an enumerator is added.

Mon, May 21, 9:52 AM

Fri, May 18

rsmith added a comment to D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages..

Any final comments on this?

Fri, May 18, 5:45 PM
rsmith added a comment to D33987: [MergeICmps][WIP] Initial MergeICmps prototype.

This change causes the compiler to crash under -fno-builtin mode, presumably because it's not checking that there is a memcmp builtin library function available for use.

Fri, May 18, 5:32 PM
rsmith added a comment to D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable..

What when compiler has __builtin_operator_new, __builtin_operator_delete? If I build libc++ tests with recent Clang which has these builtins and run tests with libc++.dylib from old Darwin, there are no linkage errors. Should we define __cpp_aligned_allocation in this case?

Fri, May 18, 5:06 PM
rsmith added a comment to D47092: downgrade strong type info names to weak_odr linkage.

I'm pretty sure this isn't actually a violation of LLVM's linkage rules as they are described in LangRef... at least, my understanding is that "equivalent" doesn't imply anything about linkage. (If this should be a violation, we need to clarify the rules.)

Fri, May 18, 4:55 PM
rsmith added a comment to D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable..

Hmm, perhaps our strategy for handling aligned allocation on Darwin should be revisited. We shouldn't be defining __cpp_aligned_allocation if we believe it doesn't work -- that will break code that uses aligned allocation where available and falls back to something else where it's unavailable.

Fri, May 18, 4:04 PM
rsmith added a comment to D46665: [Itanium] Emit type info names with external linkage..

Please see https://reviews.llvm.org/D47092 for the external -> weak_odr change.

Fri, May 18, 3:28 PM
rsmith created D47092: downgrade strong type info names to weak_odr linkage.
Fri, May 18, 3:26 PM
rsmith accepted D47084: Maintain PS4 ABI compatibility.
Fri, May 18, 2:10 PM
rsmith committed rL332760: Revert r332470 (and corresponding tests in r332492)..
Revert r332470 (and corresponding tests in r332492).
Fri, May 18, 1:22 PM
rsmith committed rC332760: Revert r332470 (and corresponding tests in r332492)..
Revert r332470 (and corresponding tests in r332492).
Fri, May 18, 1:22 PM

Thu, May 17

rsmith added a comment to D45898: [SemaCXX] Mark destructor as referenced .

As it happens, the C++ committee fixed the language wording hole here very recently. The new rule can be found here: http://wg21.link/p0968r0#2227
In summary: we should to consider the destructor for all elements of the aggregate to be potentially-invoked.

Thu, May 17, 7:49 PM

Wed, May 16

rsmith added a comment to D46665: [Itanium] Emit type info names with external linkage..

What exactly is the asan odr checker actually checking for? The distinction between common/linkonce_odr/linkonce/weak_odr/weak only affects IR optimizers, not code generation.

Wed, May 16, 6:58 PM
rsmith added a comment to D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal.

The policy certainly seems designed around the CLI use case. For serialized diagnostics, it would make sense to either serialize the snippet or enough information that the snippet can be reconstructed. And if that can't be done, or fails to satisfy some other use case, then we should discuss how we proceed -- for instance, we could consider having different diagnostic messages for the case where we have a snippet and for the case where we do not.

Right. There are places in the IDE where there is a condensed view of all diagnostics (like a Vim location list), and others where the diagnostics are shown inline with the sources. I think what we want is an optional auxiliary record/field in a diagnostic with that contains context for when the source context is missing, and then the IDE can choose which to display. It's optional because most diagnostics are good enough as is for location lists.

Wed, May 16, 6:32 PM · Restricted Project
rsmith accepted D46782: [CUDA] Allow "extern __shared__ Foo foo[]" within anon. namespaces..

Thanks, looks great.

Wed, May 16, 6:30 PM
rsmith accepted D46993: [CUDA] Make std::min/max work when compiling in C++14 mode with a C++11 stdlib..

LGTM, thanks!

Wed, May 16, 6:27 PM
rsmith added a comment to D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal.

We reconsidered this in light of the policy - thanks for pointing that out Richard!
Just to be sure that I understand it - the policy is meant for CLI and not serialized diagnostics, right?

Wed, May 16, 6:25 PM · Restricted Project
rsmith added a comment to D46665: [Itanium] Emit type info names with external linkage..

The only difference between weak_odr and linkonce_odr is that the LLVM optimizers can discard linkonce_odr globals. From your description, you want to remove the odr-ness, by changing the linkage to "linkonce", I think?

Wed, May 16, 6:14 PM
rsmith added a comment to D46665: [Itanium] Emit type info names with external linkage..

I believe static and dynamic linkers — at least on ELF and Mach-O — will always drop weak symbols for strong ones. Now, I think that isn't LLVM's posted semantics for linkonce_odr, but to me that means that LLVM's semantics are inadequate, not that we should decline to take advantage of them.

If we can't rely on that, it probably means that the type name symbol for class types always has to be linkonce_odr, even if it's for a type with a key function.

Wed, May 16, 5:33 PM
rsmith added a comment to D46665: [Itanium] Emit type info names with external linkage..

This is causing ASan's ODR violation detector to fire. I think the problematic case looks like this:

Wed, May 16, 3:17 PM

Tue, May 15

rsmith added a comment to D46919: [libclang] Deprecate CXPrintingPolicy_IncludeTagDefinition.

The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c

Tue, May 15, 6:20 PM
rsmith committed rL332425: Fix 32-bit buildbots..
Fix 32-bit buildbots.
Tue, May 15, 6:12 PM
rsmith committed rC332425: Fix 32-bit buildbots..
Fix 32-bit buildbots.
Tue, May 15, 6:11 PM
rsmith accepted D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template..

Looks good. It might be beneficial to do the lookup twice in this case and then issue a diagnostic if the results differ, but given how rarely this is happening, it doesn't really seem worthwhile.

Tue, May 15, 3:49 PM
rsmith committed rC332401: Don't produce a redundant "auto type is incompatible with C++98" on every….
Don't produce a redundant "auto type is incompatible with C++98" on every…
Tue, May 15, 2:31 PM
rsmith committed rL332401: Don't produce a redundant "auto type is incompatible with C++98" on every….
Don't produce a redundant "auto type is incompatible with C++98" on every…
Tue, May 15, 2:31 PM
rsmith added a comment to D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets.

One way we could deal with this is by adding an attribute to the compiler to indicate "the const is a lie", that we can apply to std::pair::first, with the semantics being that a top-level const is ignored when determining the "real" type of the member (and so mutating the member after a const_cast has defined behavior). This would apply to all std::pairs, not just the node_handle case, but in practice we don't optimize on the basis of a member being declared const *anyway*, so this isn't such a big deal right now.

I'm a fan of this idea, this would also have the bonus of fixing some pre-existing type-punning between pair<const K, V> and pair<K, V> (see hash_value_type and value_type in <unordered_map> and <map>).

This was supposed to be taken care of by [class.mem]/21 (common initial sequence), but that foundered on "standard-layout".

Tue, May 15, 10:46 AM

Mon, May 14

rsmith added a comment to D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal.

This would violate our guidelines for diagnostic messages; see https://clang.llvm.org/docs/InternalsManual.html#the-format-string

Mon, May 14, 4:19 PM · Restricted Project
rsmith accepted D45465: [AST] Fix printing tag decl groups in decl contexts.

Looks good, thanks.

Mon, May 14, 4:09 PM
rsmith added a comment to D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets.

How do you deal with the fact that node handles for map-like types need to be able to mutate a const member of a pair in-place? Are you assuming/hoping the compiler won't do bad things to the program as a result, or is there some reason why it would be unlikely to do so / disallowed from doing so? If you're just casting pair<const K, V> to / from pair<K, V> (and I think that's what's happening), there is a very real risk that struct-path-sensitive TBAA will decide that pair<const K, V>::first and pair<K, V>::first cannot alias (and likewise for ::second) and then miscompile code using libc++'s node pointers.

Mon, May 14, 3:43 PM
rsmith accepted D46446: [c++17] Fix assertion on synthesizing deduction guides after a fatal error..

LGTM, thanks!

Mon, May 14, 2:43 PM
rsmith added inline comments to D45465: [AST] Fix printing tag decl groups in decl contexts.
Mon, May 14, 2:40 PM
rsmith added a comment to D46846: [AST] Fix loss of enum forward decl from decl context.

Clang's current behavior is observably wrong in MS compatibility mode. For example:

Mon, May 14, 2:29 PM
rsmith committed rC332291: Fix regression in r332076..
Fix regression in r332076.
Mon, May 14, 1:56 PM
rsmith committed rL332291: Fix regression in r332076..
Fix regression in r332076.
Mon, May 14, 1:56 PM
rsmith committed rL332286: PR37450: Fix bug that disabled some type checks for variables with deduced….
PR37450: Fix bug that disabled some type checks for variables with deduced…
Mon, May 14, 1:19 PM
rsmith committed rC332286: PR37450: Fix bug that disabled some type checks for variables with deduced….
PR37450: Fix bug that disabled some type checks for variables with deduced…
Mon, May 14, 1:18 PM
rsmith added a comment to D40218: [Clang] Add __builtin_launder.

Also we probably want to hold off on landing this until PR37458 is fixed, otherwise std::launder will start miscompiling code.

Mon, May 14, 12:33 PM
rsmith added a comment to D46112: Allow _Atomic to be specified on incomplete types.

Also needs some test coverage for atomic operations which aren't calls, like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; };".

Thank you for pointing this out -- that uncovered an issue where we were not properly diagnosing the types as being incomplete. I've added a new test case and rolled the contents of Sema\atomic-type.cpp (which I added in an earlier patch) into SemaCXX\atomic-type.cpp (which already existed and I missed it).

Mon, May 14, 12:01 PM
rsmith accepted D40218: [Clang] Add __builtin_launder.

LGTM with a fix (and test) for pointer-to-array-of-dynamic-class-type handling.

Mon, May 14, 11:34 AM

Sun, May 13

rsmith accepted D45093: [AST] Fix -ast-print for _Bool when have diagnostics.
Sun, May 13, 3:02 PM
rsmith added inline comments to D46112: Allow _Atomic to be specified on incomplete types.
Sun, May 13, 2:57 PM
rsmith added inline comments to D46805: If some platforms do not support an attribute, we should exclude the platform.
Sun, May 13, 2:39 PM

Sat, May 12

rsmith added inline comments to D46441: [clang][CodeGenCXX] Noalias attr for copy/move constructor arguments.
Sat, May 12, 10:21 PM · Restricted Project
rsmith added a comment to D46441: [clang][CodeGenCXX] Noalias attr for copy/move constructor arguments.

Please notice, that this is not the only place where the position of the this pointer is assumed to always be first. As an example, another such place is here.

Sat, May 12, 10:11 PM · Restricted Project
rsmith added a comment to D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages..

Thanks, this is great.

Sat, May 12, 9:52 PM
rsmith added inline comments to D46112: Allow _Atomic to be specified on incomplete types.
Sat, May 12, 7:58 PM

Fri, May 11

rsmith added a comment to D46782: [CUDA] Allow "extern __shared__ Foo foo[]" within anon. namespaces..

Justin and I looked at a few more examples, and I think we have a somewhat better understanding now. The general behavior is:

Fri, May 11, 7:44 PM
rsmith added a comment to D46767: Force the PS4 clang ABI version to 6, and add a warning if this is attempted to be overridden.

Everything old is new again. This was discussed when -fclang-abi-compat was introduced; see https://reviews.llvm.org/D36501 for the argument why this patch is the wrong way of modeling an ABI. Forcing the ClangABICompat language option as a way of "tricking" Clang into producing the PS4 ABI is a hack. The various ABI changes that -fclang-abi-compat= controls are simply part of the PS4 ABI, and that knowledge should idealistically be carried by the CodeGen (etc) code that knows about PS4, rather than by imagining that there is some other PS4 ABI that Clang would produces at version Latest, and that we're asking for a compatibility version of it.

Fri, May 11, 5:31 PM
rsmith added a comment to D46782: [CUDA] Allow "extern __shared__ Foo foo[]" within anon. namespaces..

Rather than suppressing the warning, should we instead give such variables external linkage?

Fri, May 11, 4:07 PM
rsmith added a comment to D46782: [CUDA] Allow "extern __shared__ Foo foo[]" within anon. namespaces..

Rather than suppressing the warning, should we instead give such variables external linkage?

Fri, May 11, 4:01 PM
rsmith added a comment to D45463: [AST] Print correct tag decl for tag specifier.

A few comments ago, I mentioned that IncludeTagDefinition's documentation and name is drifting farther from its functionality. Should we do something about that?

Fri, May 11, 3:49 PM
rsmith added a comment to D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages..

Thank you, I've been wanting this for ages but never got around to it :)

Fri, May 11, 1:34 PM
rsmith accepted D45463: [AST] Print correct tag decl for tag specifier.

LGTM, thanks!

Fri, May 11, 12:56 PM
rsmith committed rL332130: [libclang] Stop assuming that the internal C++ ABI ExceptionSpecificationType….
[libclang] Stop assuming that the internal C++ ABI ExceptionSpecificationType…
Fri, May 11, 12:50 PM
rsmith committed rC332130: [libclang] Stop assuming that the internal C++ ABI ExceptionSpecificationType….
[libclang] Stop assuming that the internal C++ ABI ExceptionSpecificationType…
Fri, May 11, 12:50 PM
rsmith added a comment to D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type.

Thanks, I think this can be simplified a bit but this looks like the right fix.

Fri, May 11, 12:00 PM

Thu, May 10

rsmith committed rC332076: Improve diagnostics and error recovery for template name lookup..
Improve diagnostics and error recovery for template name lookup.
Thu, May 10, 7:49 PM
rsmith committed rL332076: Improve diagnostics and error recovery for template name lookup..
Improve diagnostics and error recovery for template name lookup.
Thu, May 10, 7:49 PM
rsmith added inline comments to D45463: [AST] Print correct tag decl for tag specifier.
Thu, May 10, 7:39 PM
rsmith added inline comments to D45680: [C++2a] Implement operator<=> Part 2: Operator Rewritting and Overload Resolution..
Thu, May 10, 4:37 PM
rsmith added inline comments to D46241: [CodeGen] Recognize more cases of zero initialization.
Thu, May 10, 2:53 PM
rsmith added inline comments to D46684: [Frontend] Don't skip function body when the return type is dependent on the template parameter..
Thu, May 10, 7:46 AM

Wed, May 9

rsmith accepted D43320: Allow dllimport non-type template arguments in C++17.
Wed, May 9, 6:08 PM
rsmith added inline comments to D46665: [Itanium] Emit type info names with external linkage..
Wed, May 9, 5:05 PM