Page MenuHomePhabricator

erichkeane (Erich Keane)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 28 2016, 8:37 AM (313 w, 1 d)

Recent Activity

Today

erichkeane updated the diff for D126907: Deferred Concept Instantiation Implementation Take 2.

All but the 1 comment from @ChuanqiXu fixed, not sure what to do about the 'info'.

Wed, Jun 29, 8:05 AM · Restricted Project
erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

All tests pass now, I was able to get the template-template checks working correctly, and it passes all the tests I have available. @ChuanqiXu if you could review/run what tests you have, it would be greatly appreciated.

I've tested some of our workloads and libunifex (I know it contains a lot of uses for concept). And all the tests passed now. So it looks like it wouldn't cause regression failure.

The implementation basically looks good to me. (This is really large and I can't be sure I don't miss something). Only some minor issues remained.

Wed, Jun 29, 8:03 AM · Restricted Project

Yesterday

erichkeane added a comment to D128750: [c++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions.

Is there any chance you can validate this against https://reviews.llvm.org/D126907 as well? We touch similar code, and I'm intending to get that committed in the near future. Otherwise, after a quick look, I don't see anything particualrly bad, other than a lack of release notes and update of www-status.

Tue, Jun 28, 12:08 PM · Restricted Project, Restricted Project
erichkeane updated the diff for D126907: Deferred Concept Instantiation Implementation Take 2.

Rebased!

Tue, Jun 28, 11:59 AM · Restricted Project
erichkeane updated the diff for D126907: Deferred Concept Instantiation Implementation Take 2.

All tests pass now, I was able to get the template-template checks working correctly, and it passes all the tests I have available. @ChuanqiXu if you could review/run what tests you have, it would be greatly appreciated.

Tue, Jun 28, 8:54 AM · Restricted Project
erichkeane added a comment to D128351: [clang] missing outer template levels when checking template constraints.

So I don't think this is the right fix for this... But I DID check and see that the test case seems to be fixed here: https://reviews.llvm.org/D126907 , which does a lot of rework for this sort of thing.

Tue, Jun 28, 6:05 AM · Restricted Project, Restricted Project

Mon, Jun 27

erichkeane added inline comments to D108469: Improve handling of static assert messages..
Mon, Jun 27, 12:45 PM · Restricted Project, Restricted Project, Restricted Project
erichkeane added inline comments to D108469: Improve handling of static assert messages..
Mon, Jun 27, 12:44 PM · Restricted Project, Restricted Project, Restricted Project
erichkeane added inline comments to D108469: Improve handling of static assert messages..
Mon, Jun 27, 12:36 PM · Restricted Project, Restricted Project, Restricted Project
erichkeane added a comment to D108469: Improve handling of static assert messages..

I'm generally OK with this. BUT Aaron/JF/Tom should review it.

Mon, Jun 27, 12:12 PM · Restricted Project, Restricted Project, Restricted Project
erichkeane added inline comments to D108469: Improve handling of static assert messages..
Mon, Jun 27, 9:38 AM · Restricted Project, Restricted Project, Restricted Project
erichkeane added inline comments to D108469: Improve handling of static assert messages..
Mon, Jun 27, 8:24 AM · Restricted Project, Restricted Project, Restricted Project

Wed, Jun 22

erichkeane added a comment to D127812: [AArch64] Function multiversioning support added..

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

Thank you for fair concern, "target_clones" for AArch64 has different format, semantic, e.g. "default" is not required. Therefore it diverges with X86 in these parts.

Is it *necessary* that it diverges like this? (Is there some published standards document you're trying to conform to, is there an implementation difficulty with not diverging, something else?)

In ACLE there are a few rules/behaviour documented around what should be the behaviour of the "default" for example. Making for example "default" optional hopefully makes the adation of multi versioning seamless as possible. Compilers won't support from day one these attributes therefore the goal was to make the whole addition of a multi versioned function as less distributive as possible while the code is still compiled with older compilers too.

// this is the original code
// mandating __attribute__ ((target ("default"))) would not work with the today's compilers
void foo(){}

// new backward compatible code
#ifdef __ARM_FEATURE_FUNCTION_MULTI_VERSIONING
__attribute__ ((target_version("fancy_feature")))
void foo(){}
#endif

if the "default" is not mandated here, it felt not right to mandate it for the target_clones either.

"target" attribute has been already used and supported on AArch64 in a different sense, like target("arm"), target("dotprod"), target("branch-protection=bti"). The intention of creating new "target_version" attribute is not to overlap with that. It also has different format, mangling and semantic, e.g. treating function without attribute as "default", and option to disable attribute droping function multi versions. Do these explanations dispel your concern?

Do I understand correctly that AArch64 was using an attribute named target which does not behave like the attribute with the same name in GCC and Clang on other architectures, and now you'd like to introduce a new attribute which does behave like target but under a different name? If I have that correct, I don't think that's reasonable -- there's far too much possibility for confusion with that situation already, and adding a new attribute only increases the confusion. I'm not certain where the technical debt came from, but we shouldn't increase the burden on users here; I think target and target_clones should be made to work consistently across architectures if at all possible.

Your understanding is correct. target attribute has two usage model. One is just redefine the to be used codegen options, this is used already widely for Arm and AArch64. The other use of the target attribute is the multi versioning and the rational for the target_version attribute is the easier distinction between the two usage mode, also not to break any code out there by changing the behaviour of an attribute.

Wed, Jun 22, 12:10 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
erichkeane accepted D127487: [Sema] Fix assertion failure when instantiating requires expression.

Ah, i see! Thanks for the explanation!

Wed, Jun 22, 9:03 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D127487: [Sema] Fix assertion failure when instantiating requires expression.
Wed, Jun 22, 6:27 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

Great progress!

Note that the failure comes down to:

template<typename T> concept C = T::f();
template<template<C> class P> struct S1{};
template<C> struct X{};
S1<X> s11;

and requires the -frelaxed-template-template-args flag:

[ekeane1@scsel-clx-24 build]$  ./bin/clang -cc1 -std=c++20 temp.cpp -frelaxed-template-template-args
temp.cpp:5:4: error: template template argument 'X' is more constrained than template template parameter 'P'
S1<X> s11;
   ^
temp.cpp:3:29: note: 'P' declared here
template <template<C> class P> struct S1{};
                            ^
temp.cpp:4:20: note: 'X' declared here
template<C> struct X{};
                   ^
1 error generated.

As far as I could tell, we could omit the diagnostic by deleting https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/include/clang/Sema/SemaConcept.h#L45-L53

This change is obsolutely wrong but it shows the bug comes from ParameterMapping so we could locate https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/lib/Sema/SemaConcept.cpp#L723 further.

Wed, Jun 22, 6:05 AM · Restricted Project

Tue, Jun 21

erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

Note that the failure comes down to:

Tue, Jun 21, 11:39 AM · Restricted Project
erichkeane updated the diff for D126907: Deferred Concept Instantiation Implementation Take 2.

As promised, got it down to the 1 failure, and added tests for @ChuanqiXu and the std::vector example. That all seems to work, just a problem with the CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp test left.

Tue, Jun 21, 11:30 AM · Restricted Project

Fri, Jun 17

erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

Made good progress today, but got trapped down the CheckDeducedArgs path for quite a bit longer with partial specializations. I have all of the crashes I know of 'fixed', but have a few tests that still fail for unknown reasons. Because of that, i don't have anything I can really upload, so I'll have to do so mid-next-week.

Fri, Jun 17, 1:19 PM · Restricted Project
erichkeane accepted D126194: [Concepts] Implement overload resolution for destructors (P0848).
Fri, Jun 17, 6:18 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In ::CheckConstraintSatisfaction, the depth of RawCompletionToken is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.

The first though was that we should set the depth of RawCompletionToken to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of RawCompletionToken is 0 indeed. What is different that time is the depth of corresponding MultiLevelTemplateArgumentList is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.

So I feel like what we should do here is in Sema::CheckInstantiatedFunctionTemplateConstraints, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.


Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.

I don't know of the 'another idea'? We have to do instantiation before checking, and if we do it too early, we aren't doing the deferred instantiation. And the problem is that the RawCompletionToken uses a concept to refer to 'itself'. However, this version of 'itself' isn't valid, since the 'depth' is wrong once it tries to instantiate relative to the 'top level'. However, this IS happening during instantiation?

My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT happen at the Sema level (but at the instantiator level), since it ends up catching things it shouldn't? I tried it and have a handful of failures of trying to check uninstantiated constraints, but I've not dug completely into it.

Yeah, we have to do instantiation before checking. My point is that it looks like we're doing another instantiation when we check the concepts. And my point is that if it we should avoid the second instantiation.

Fri, Jun 17, 6:08 AM · Restricted Project

Thu, Jun 16

erichkeane added a comment to D127593: [clang] Fix trivially copyable for copy constructor and copy assignment operator.

I did try this locally and the built test binaries seem to crash with a segmentation fault. I run one of them in lldb to check the error and I got this backtrace:

llvm-project/build on  fix-trivially-copyable-check [?⇕] via △ v3.22.3 took 3s ❯ lldb /Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp
(lldb) target create "/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp"
Current executable set to '/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp' (arm64).
(lldb) r
Process 61339 launched: '/Users/javier/Desktop/Projects/llvm-project/build/projects/openmp/runtime/test/worksharing/single/Output/omp_single_copyprivate.c.tmp' (arm64)
Process 61339 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
    frame #0: 0x0000000100362f54 libomp.dylib`::__kmp_get_global_thread_id() at kmp_runtime.cpp:201:8
   198
   199 	  /* dynamically updated stack window for uber threads to avoid get_specific
   200 	     call */
-> 201 	  if (!TCR_4(other_threads[i]->th.th_info.ds.ds_stackgrow)) {
   202 	    KMP_FATAL(StackOverflow, i);
   203 	  }
   204
Target 0: (omp_single_copyprivate.c.tmp) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
  * frame #0: 0x0000000100362f54 libomp.dylib`::__kmp_get_global_thread_id() at kmp_runtime.cpp:201:8
    frame #1: 0x00000001003b8c60 libomp.dylib`void __kmp_release_template<kmp_flag_64<false, true> >(flag=0x000000016fdff0b8) at kmp_wait_release.h:790:39
    frame #2: 0x00000001003b8c20 libomp.dylib`::__kmp_release_64(flag=0x000000016fdff0b8) at kmp_wait_release.cpp:25:46
    frame #3: 0x00000001003798d0 libomp.dylib`__kmp_reap_thread(thread=0x0000000101010a40, is_root=0) at kmp_runtime.cpp:6146:9
    frame #4: 0x000000010037497c libomp.dylib`__kmp_internal_end() at kmp_runtime.cpp:6334:7
    frame #5: 0x00000001003745ec libomp.dylib`::__kmp_internal_end_library(gtid_req=-1) at kmp_runtime.cpp:6498:3
    frame #6: 0x0000000100374258 libomp.dylib`::__kmp_internal_end_atexit() at kmp_runtime.cpp:6115:3
    frame #7: 0x0000000100374218 libomp.dylib`::__kmp_internal_end_dtor() at kmp_runtime.cpp:6083:3
    frame #8: 0x0000000189359dc4 libsystem_c.dylib`__cxa_finalize_ranges + 464
    frame #9: 0x0000000189359b68 libsystem_c.dylib`exit + 44
    frame #10: 0x000000018947aec4 libdyld.dylib`dyld4::LibSystemHelpers::exit(int) const + 20
    frame #11: 0x00000001000150d4 dyld`start + 592
(lldb)

Not sure how to proceed from here though... I'm really not familiar with OpenMP

Thu, Jun 16, 12:50 PM · Restricted Project, Restricted Project, Restricted Project
erichkeane added a comment to D126194: [Concepts] Implement overload resolution for destructors (P0848).

I probably need to spend more time on this at one point, but first look seemed fine to me. I think the approach is about right, and the solution is there.

Thu, Jun 16, 12:01 PM · Restricted Project, Restricted Project
erichkeane accepted D127933: [clang] Don't emit IFUNC when targeting Fuchsia.
Thu, Jun 16, 6:06 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

From what I can see, the crash reason would be the mismatch depth and the setting of MultiLevelTemplateArgumentList. In ::CheckConstraintSatisfaction, the depth of RawCompletionToken is 0, while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost template argument incorrectly (which is a template pack in the example) in TransformTemplateTypeParmType.

The first though was that we should set the depth of RawCompletionToken to 1 correctly. But I felt this might not be good later since in the normal process of template instantiation (without concepts and constraints), the depth of RawCompletionToken is 0 indeed. What is different that time is the depth of corresponding MultiLevelTemplateArgumentList is 1. So it looks like the process is constructed on the fly. It makes sense for the perspective of compilation speed.

So I feel like what we should do here is in Sema::CheckInstantiatedFunctionTemplateConstraints, when we are computing the desired MultiLevelTemplateArgumentList, we should compute a result with depth of 1 in the particular case.


Another idea is to not do instantiation when we're checking constraints. But I need to think more about this.

Thu, Jun 16, 5:59 AM · Restricted Project

Wed, Jun 15

erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

See here: https://github.com/llvm/llvm-project/issues/55673

This might be the same issue, the crash at least looks the same.

Wed, Jun 15, 12:57 PM · Restricted Project
erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

See here: https://github.com/llvm/llvm-project/issues/55673

Wed, Jun 15, 8:24 AM · Restricted Project
erichkeane added a comment to D127812: [AArch64] Function multiversioning support added..

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

Wed, Jun 15, 6:54 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

I finally found some time to look at the crash. Although I haven't get an idea, I found it crash at the following one too:

template <typename T>
concept Constraint = true;

template <typename... Signatures>
class completion_handler_async_result {
public:
    template <typename Initiation, Constraint RawCompletionToken>
    static void initiate(Initiation&& initiation, RawCompletionToken&& token);
};

template <typename... Signatures>
concept Constraint2 =
    requires() {
        completion_handler_async_result<Signatures...>::initiate(0, 0);
    };

template <typename T>
void use(int F)
    requires Constraint2<int>
{}

The crash log is:

clang/lib/AST/ExprConstant.cpp:15083: bool clang::Expr::EvaluateAsConstantExpr(clang::Expr::EvalResult&, const clang::ASTContext&, clang::Expr::ConstantExprKind) const: Assertion `!isValueDependent() && "Expression evaluator can't be called on a dependent expression."' failed.

I am not sure if they are the same problem.

Wed, Jun 15, 6:35 AM · Restricted Project
erichkeane added inline comments to D127626: [docs] Add document "Debugging C++ Coroutines".
Wed, Jun 15, 6:33 AM · debug-info, Restricted Project
erichkeane added a comment to D127593: [clang] Fix trivially copyable for copy constructor and copy assignment operator.

Thank you! LGTM. I see the CI still has those OpenMP failures - could you try to rebase on main to get it green?

@erichkeane is this good to go? Should I ping some ABI people about this?

Wed, Jun 15, 6:02 AM · Restricted Project, Restricted Project, Restricted Project

Tue, Jun 14

erichkeane added inline comments to D127759: [Diagnostic] Clarify -Winfinite-recursion message.
Tue, Jun 14, 12:48 PM · Restricted Project, Restricted Project
erichkeane added inline comments to D127759: [Diagnostic] Clarify -Winfinite-recursion message.
Tue, Jun 14, 12:30 PM · Restricted Project, Restricted Project
erichkeane added inline comments to D127759: [Diagnostic] Clarify -Winfinite-recursion message.
Tue, Jun 14, 10:49 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D127626: [docs] Add document "Debugging C++ Coroutines".
Tue, Jun 14, 7:18 AM · debug-info, Restricted Project
erichkeane added a comment to D127626: [docs] Add document "Debugging C++ Coroutines".

Did another quick read through and made a handful of 'nit' level suggestions. Since much of it is my words, I'd love it if someone else could take a read through as a double-check.

Tue, Jun 14, 6:48 AM · debug-info, Restricted Project
erichkeane accepted D127201: [clang] Add tests for statement expression in initializers.
Tue, Jun 14, 5:50 AM · Restricted Project, Restricted Project

Mon, Jun 13

erichkeane added a comment to D127593: [clang] Fix trivially copyable for copy constructor and copy assignment operator.

I don't think the OpenMP tests are failing because of this PR, they are a bit flaky sometimes.

@erichkeane - Since we haven't branched clang15 yet, I think it's Ok not to add the Ver15 value to the ABI options?

Mon, Jun 13, 1:18 PM · Restricted Project, Restricted Project, Restricted Project
erichkeane added a comment to D127626: [docs] Add document "Debugging C++ Coroutines".

The organization and content of the document are quite good, I learned quite a lot, so thank you! I found the prose itself a bit difficult to read, so did my best to reword in each section as I could. Feel free to take/modify/review whatever you'd like of it.

Mon, Jun 13, 10:51 AM · debug-info, Restricted Project
erichkeane added a comment to D127201: [clang] Add tests for statement expression in initializers.

The command line on one of the tests is a little strange to me, and I don't see the need for the -std=gnu11 on either, but the test content itself looks fine enough to me.

Mon, Jun 13, 7:58 AM · Restricted Project, Restricted Project
erichkeane added a comment to D127593: [clang] Fix trivially copyable for copy constructor and copy assignment operator.

Hi Javier, thank you for submitting this patch!

As far as I could tell, this patch implements the CWG2171 defect report from 2016: https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2171. That means that you'll have to add a test inside clang/test/CXX/drs/dr21xx.cpp to make sure it shows on the DRs status page.

This change is also a large ABI break, which means it has to be considered carefully. I think the best approach here would be to revert to the previous behavior when the -fclang-abi-compat flag is used for clang <= 14, but I'm not sure about this. I would like the approval of someone with more ABI experience here.

The code itself and the tests look good! If you'll need help with the changes I mentioned please let me know.

Mon, Jun 13, 7:37 AM · Restricted Project, Restricted Project, Restricted Project
erichkeane added inline comments to D127518: [Diagnostics] Fix inconsistent shift-overflow warnings in C++20.
Mon, Jun 13, 7:34 AM · Restricted Project, Restricted Project

Fri, Jun 10

erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

So from what I can tell, the problem is evaluating Constraint on initiate, and trying to find the 'right' argument from the MultiLevelTemplateArgumentList for RawCompletionToken. The problem is that the RawCompletionToken, as a template parameter, gets 'filled in to' the Concept relative to initiate instead of relative to the "root".

Fri, Jun 10, 11:08 AM · Restricted Project
erichkeane added inline comments to D127487: [Sema] Fix assertion failure when instantiating requires expression.
Fri, Jun 10, 9:49 AM · Restricted Project, Restricted Project
erichkeane added a comment to D127487: [Sema] Fix assertion failure when instantiating requires expression.

I've checked that the change works fine on top of D126907. The bug is still there after D126907 and gets fixed by this.
Also, the merge conflict is actually minimal, no code changes intersect.

Fri, Jun 10, 8:55 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D127487: [Sema] Fix assertion failure when instantiating requires expression.
Fri, Jun 10, 7:29 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D126960: [clang][sema] Unary not boolean to int conversion.
Fri, Jun 10, 6:27 AM · Restricted Project, Restricted Project
erichkeane added a comment to D127487: [Sema] Fix assertion failure when instantiating requires expression.

I'm not quite understanding this yet, so I'll have to take another look early next week. However, I AM intending to get https://reviews.llvm.org/D126907 committed in the next week or so. Could you perhaps see how it interacts with that? Its a sizable, multi-month project that I'd like to make sure doesn't get stuck in a rebase-loop again.

Fri, Jun 10, 6:03 AM · Restricted Project, Restricted Project

Thu, Jun 9

erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

I was able to reduce it today. BUT didn't get a chance to debug:

Thu, Jun 9, 12:45 PM · Restricted Project
erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

All of the comments from @ChuanqiXu done, thank you so much!

As far as the crash you're seeing: That is typically a 'depth mismatch' kinda thing. They sadly only show up when the types of the template arguments are different (that is, pack vs integral vs type), but I thought I got all of them with examples.

I'll keep hunting for more during the day today, but any amount of reproducer you can provide (even if not minimized) would be incredibly helpful!

Oh, if a not minimized reproducer is desired, you could find it at: https://github.com/alibaba/async_simple. It should crash when we run 'make' in build directory. (The version of libstdc++ is 10.2 in my environment)

I would try to make a reduced example in the next week if we don't go on smoothly.

Thu, Jun 9, 6:26 AM · Restricted Project

Wed, Jun 8

erichkeane accepted D126944: [Clang] Fix memory leak due to TemplateArgumentListInfo used in AST node..

LGTM! Thanks for this!

Wed, Jun 8, 9:52 AM · Restricted Project, Restricted Project, Restricted Project
erichkeane updated the diff for D126907: Deferred Concept Instantiation Implementation Take 2.

All of the comments from @ChuanqiXu done, thank you so much!

Wed, Jun 8, 7:13 AM · Restricted Project
erichkeane added inline comments to D126907: Deferred Concept Instantiation Implementation Take 2.
Wed, Jun 8, 7:10 AM · Restricted Project
erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

Oh, the diff part is small really. The change looks good to me and I've tested it for libcxx.

But it crashed at "clang/lib/Sema/SemaTemplateInstantiate.cpp:1852: clang::QualType {anonymous}::TemplateInstantiator::TransformTemplateTypeParmType(clang::TypeLocBuilder&, clang::TemplateTypeParmTypeLoc): Assertion `Arg.getKind() == TemplateArgument::Type && "Template argument kind mismatch"' failed" in an internal library. I am trying to get a reduced example. I would suggest to do more testing with other workloads (with libstdc++ >= 10)

Wed, Jun 8, 6:02 AM · Restricted Project

Tue, Jun 7

erichkeane committed rG5c3bde96250c: [CodeGen] Fix an issue when the 'extern C' replacement names broke (authored by erichkeane).
[CodeGen] Fix an issue when the 'extern C' replacement names broke
Tue, Jun 7, 11:31 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126818: Itanium ABI: Implement mangling for constrained friends.

Note we might be confused, the parens there aren't completely clear as to what your intent is.

Well, I know that I'm confused and not clear what my intent is :)

I asked the question because there appears to be an asymmetry between (temp.friend)p9 sentence 1 and (temp.friend)p9 sentence 2. Sentence 1 applies to all constrained non-template friend declarations regardless of any template argument dependence while sentence 2 applies to just a subset of constrained friend function templates; those that have some amount of template dependence. The difference impacts when mangling differences are required.

I spent some time analyzing how gcc, clang, and MSVC handle these different cases. See https://godbolt.org/z/85E5eMh3x. Search for FIXME? to find cases where I think one or more of the compilers is misbehaving or where it is unclear to me whether or how [temp.friend]p9 applies. Some highlights:

  • Some compilers diagnose some ill-formed cases when parsing the class template, others don't until the class template is instantiated. Not surprising.
  • All of the compilers reject non-template friend function definitions with non-dependent constraints due to duplicate definitions, presumably in violation of [temp.friend]p9; see the ff2() example.
  • All of the compilers reject non-template friend function definitions with dependent constraints due to duplicate definitions, presumably in violation of [temp.friend]p9; see the ff6() example.
  • Name mangling is currently insufficient to differentiate (otherwise) non-dependent friend function templates with dependent constraints; see the fft5() and fft6() examples.
  • None of the compilers reject some cases of non-definitions that should be rejected by [temp.friend]p9; see the fft5() and fft7() examples.
Tue, Jun 7, 6:18 AM · Restricted Project
erichkeane added a comment to D126907: Deferred Concept Instantiation Implementation Take 2.

@ChuanqiXu : i was hoping you could take a look at this, since you did such a great job reviewing the rest of this (note this is mostly the same patch as the last one, just with the 'friends' stuff dealt with).

Tue, Jun 7, 6:11 AM · Restricted Project

Fri, Jun 3

erichkeane accepted D126969: Allow use of an elaborated type specifier in a _Generic association in C++.
Fri, Jun 3, 10:08 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126969: Allow use of an elaborated type specifier in a _Generic association in C++.

This mostly/all seems reasonable to me, and the diagnostics, while not perfect, are IMO a vast improvement.

Fri, Jun 3, 9:17 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126818: Itanium ABI: Implement mangling for constrained friends.

I'm a bit uneasy that this is implementing something that's not yet been accepted into the Itanium ABI document. That runs the risk of requiring an ABI break if the Itanium document changes directions before finalizing. Also, what should we be doing for the Microsoft mangling, or do we already handle that properly?

Fri, Jun 3, 7:15 AM · Restricted Project
erichkeane added a comment to D126818: Itanium ABI: Implement mangling for constrained friends.

I wonder if I'm reading (temp.friend)p9` sentence 2 correctly. Which of these should it be parsed as?

  1. A (friend function template with a constraint) that depends on a template parameter from an enclosing template shall be a definition.
  2. A friend function template (with a constraint that depends on a template parameter from an enclosing template) shall be a definition.

I'm guessing that the first interpretation is the intent.

Fri, Jun 3, 5:59 AM · Restricted Project

Thu, Jun 2

erichkeane requested review of D126907: Deferred Concept Instantiation Implementation Take 2.
Thu, Jun 2, 12:01 PM · Restricted Project
erichkeane added a comment to D125402: [clang][diag] warn if function returns class type by-const-value.

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

The 'Simple' version might not be quite right... That is implemented as:

bool hasSimpleMoveConstructor() const {
   return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
          !data().DefaultedMoveConstructorIsDeleted;
 }

So this would still warn about user-defined move constructors.

Good catch!

HOWEVER, I might suggest hasMoveConstructor() && !defaultedMoveConstructorIsDeleted() for the ctor test. There is similar storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so you might need to add a function to expose it in DeclCXX.h.

I think that might still not be correct because there could be a user-provided move constructor that's explicitly deleted. (So it has a move constructor, but the defaulted one is not deleted because it's user provided.)

I don't think that is possible? I think 'defaultedMoveConstructor' includes the user typing StructName(StructName&&) = delete;.

Note that out of line deleted implementations are prohibited: https://godbolt.org/z/MTK4jGa57 So we don't have to worry about that.

Thu, Jun 2, 11:00 AM · Restricted Project, Restricted Project
erichkeane added a comment to D125402: [clang][diag] warn if function returns class type by-const-value.

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

The 'Simple' version might not be quite right... That is implemented as:

bool hasSimpleMoveConstructor() const {
   return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
          !data().DefaultedMoveConstructorIsDeleted;
 }

So this would still warn about user-defined move constructors.

Good catch!

HOWEVER, I might suggest hasMoveConstructor() && !defaultedMoveConstructorIsDeleted() for the ctor test. There is similar storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so you might need to add a function to expose it in DeclCXX.h.

I think that might still not be correct because there could be a user-provided move constructor that's explicitly deleted. (So it has a move constructor, but the defaulted one is not deleted because it's user provided.)

Thu, Jun 2, 10:42 AM · Restricted Project, Restricted Project
erichkeane added a comment to D125402: [clang][diag] warn if function returns class type by-const-value.

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. hasSimpleMoveConstructor() and hasSimpleMoveAssignment() might be a better approach.

Thu, Jun 2, 10:35 AM · Restricted Project, Restricted Project
erichkeane planned changes to D126818: Itanium ABI: Implement mangling for constrained friends.

Note this was mentioned as a 'must also do' in my discussion on deferred constraints with @rsmith, however as this already fails, I don't see this as a prerequisite. However, I think this is easy enough to just do.

THAT SAID: The proposal on the itanium-abi github wasn't particularly clear as to which of the following three options were the 'right' answer. I'm hoping @rjmccall can decide whether this is acceptable:

Option 1: Change the mangling of ALL friend functions. I determined this is likely an ABI break on otherwise 'settled' code, and figured we wouldn't want to do this.

Option 2: Change the mangling for ALL constrained friend functions. This seemed easy enough to do, AND changes the mangling for things only covered by 'experimental' concepts support. So I think changing the ABI for current things is OK here.

Option 3: Change the mangling for constrained friend functions that fall under temp.friend p9. The only difference between this and Option2 is that this would need to check the 'template friend function depends on containing scope'. I don't believe this is necessary so long as we are consistent with it. Presumably this could end up with multiple symbols for functions that are the 'same declaration' by rule in separate translation units, such as:

struct Base{};
template<int N>
struct S {
  template<typename U>
  friend void func(Base&) requires(U::value) {}
};
void uses() {
  S<1> s;
  S<2> s2; // This is an error during Sema, since 'func' is a duplicate here.
  func(s); // all S<N>s SHOULD end up with the same declaration, but in different TUs they might end up with different copies here?
}

In typing this comment up, I believe I got the 'option' wrong here. Should I have gone with #3 above? I'm going to think about it overnight (and hope you reviewer folks can chime in!) and get to it in the morning.

Thu, Jun 2, 6:08 AM · Restricted Project
erichkeane added a comment to D125802: Fix std::has_unique_object_representations for _BitInt types with padding bits.

Wow, thank you for the fantastic sleuthing! It seems that declaration is nine years old, so I'm surprised the leak is only being discovered now and as part of this particular test case.

I don't have a particularly easy way to test this locally at the moment -- do you know if switching VarTemplateSpecializationDecl::TemplateArgsInfo to be a ASTTemplateArgumentListInfo solves the issue for you?

Thu, Jun 2, 5:59 AM · Restricted Project, Restricted Project

Wed, Jun 1

erichkeane updated subscribers of D126818: Itanium ABI: Implement mangling for constrained friends.

Note this was mentioned as a 'must also do' in my discussion on deferred constraints with @rsmith, however as this already fails, I don't see this as a prerequisite. However, I think this is easy enough to just do.

Wed, Jun 1, 1:31 PM · Restricted Project
erichkeane requested review of D126818: Itanium ABI: Implement mangling for constrained friends.
Wed, Jun 1, 1:23 PM · Restricted Project

Tue, May 31

erichkeane accepted D126642: [Clang] NFCI: Add a new bit HasExtraBitfields to FunctionType..

I think I'm Ok with this as a 1st step for the cleanup. I think we should probably evaluate what amount of work is necessary to extract the ExtInfo out into trailing storage, depending on whether it saves us room in the FunctionProtoType and FunctionNoProtoType types.

Tue, May 31, 11:14 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126642: [Clang] NFCI: Add a new bit HasExtraBitfields to FunctionType..

Right, yeah. One thing to consider: ExtInfo was 'first', and so it got to keep the 'in bitfield bits'. However, much of what is in there is, IMO, not something that is used often enough to be worth having in there. Of the bits being used there, I think 'NoReturn' is the only one used with any regularity (other than perhaps 'this call' in Windows-32-bit machines). I wonder if we should consider moving as much of that as possible over.

That sounds sensible. Some of the fields there are also valid in FunctionNoProtoType though, and I don't believe I saw an equivalent to ExtProtoInfo for FunctionNoProtoType, is that because it's uncommon enough that it only requires changing in a handful of places? I'm not too familiar with Clang code, so I didn't look to deep into this.

Tue, May 31, 11:11 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126512: [Docs] Clarify the guideline on omitting braces.

I agree with the above, we probably need an RFC on discourse for this.

Tue, May 31, 10:54 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D126512: [Docs] Clarify the guideline on omitting braces.
Tue, May 31, 10:38 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D126512: [Docs] Clarify the guideline on omitting braces.
Tue, May 31, 9:30 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126642: [Clang] NFCI: Add a new bit HasExtraBitfields to FunctionType..

I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here.

The reason I repurposed the bit was because I previously tried adding a bit to unsigned ExtInfo : 13; but that fails

static_assert(sizeof(*this) <= 8 + sizeof(ExtQualsTypeCommonBase), "changing bitfields changed sizeof(Type)!");
Tue, May 31, 8:39 AM · Restricted Project, Restricted Project
erichkeane accepted D119646: [clang] Allow consteval functions in default arguments.

This seems innocuous enough, and seems to better reflect the intent of the standard, so LGTM.

Tue, May 31, 7:06 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126642: [Clang] NFCI: Add a new bit HasExtraBitfields to FunctionType..

I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here. Also, the 'hasBitfields' seems like the answer should just be 'no' in the case when its 'no'...

Tue, May 31, 7:01 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126512: [Docs] Clarify the guideline on omitting braces.

I'd prefer we limit the scope of this to JUST a 'do-while' loop. I think we can clarify the "if an inner statement uses braces, the outer one should too", AND special-case the do-while loop as needing braces.

Tue, May 31, 6:24 AM · Restricted Project, Restricted Project

May 24 2022

erichkeane added inline comments to D126194: [Concepts] Implement overload resolution for destructors (P0848).
May 24 2022, 6:49 AM · Restricted Project, Restricted Project
erichkeane accepted D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585.

I have a change to the release note that I'd like to see improved in SOME way, but I trust you to correct/reword as you wish. I'm still not particularly happy with the mechanism of the test, but I cannot come up with a way to cause the Semantic Analyzer to cause this.

May 24 2022, 6:39 AM · Restricted Project, Restricted Project, Restricted Project

May 23 2022

erichkeane added inline comments to D126194: [Concepts] Implement overload resolution for destructors (P0848).
May 23 2022, 8:07 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D126194: [Concepts] Implement overload resolution for destructors (P0848).
May 23 2022, 7:53 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126194: [Concepts] Implement overload resolution for destructors (P0848).

How much of P0848 is missing after this? If nothing/not much, should we update cxx_status.html as well?

May 23 2022, 6:47 AM · Restricted Project, Restricted Project
erichkeane added a comment to D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585.

Also needs some release notes.

May 23 2022, 6:35 AM · Restricted Project, Restricted Project, Restricted Project

May 20 2022

erichkeane committed rG0ef7ca86cf1e: Fix test from df2a4e to work with 32 bit windows targets. (authored by erichkeane).
Fix test from df2a4e to work with 32 bit windows targets.
May 20 2022, 6:22 AM · Restricted Project, Restricted Project
erichkeane added a comment to D119544: Deferred Concept Instantiation Implementation.

Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts. I'll likely be submitting this early monday or so, but would like to give @ChuanqiXu a chance to check out the changes.

This is the same as my 'last' committed version of this patch, plus the changes I split out into here for easier review: https://reviews.llvm.org/D125020

Sorry that I missed the ping. The change in that revision looks good to me.

I THINK what we want to do instead of trying to 're setup' the template arguments (and instead of just 'keeping' the uninstantiated expression) is to run some level of 'tree-transform' that updates the names of the template parameters (including depths/etc), but without doing lookup. This I think would fix our 'friend function' issue, and our 'comparing constraints' stuff work correctly as well (rather than storing an instantiated version, but not reusing it except for comparing them).

I agree this should be a better direction.

May 20 2022, 6:04 AM · Restricted Project, Restricted Project

May 19 2022

Herald added a reviewer for D119544: Deferred Concept Instantiation Implementation: dang.

@rsmith pointed out https://eel.is/c++draft/temp#friend-9 which I think is supposed to fix the friend function issues:

May 19 2022, 6:40 AM · Restricted Project, Restricted Project

May 18 2022

erichkeane accepted D125919: Drop qualifiers from return types in C (DR423).
May 18 2022, 12:39 PM · Restricted Project, Restricted Project
erichkeane accepted D125882: Correct the diagnostic behavior for unreachable _Generic associations in C++.

Took me a bit to get the prose, but I think I got it now. PERHAPS there is use to 'finish the thought' on the outcome there? Either way, LGTM.

May 18 2022, 6:44 AM · Restricted Project, Restricted Project
erichkeane accepted D125802: Fix std::has_unique_object_representations for _BitInt types with padding bits.
May 18 2022, 5:54 AM · Restricted Project, Restricted Project

May 17 2022

erichkeane added a comment to D125802: Fix std::has_unique_object_representations for _BitInt types with padding bits.

This likely also needs a Release Note.

May 17 2022, 9:48 AM · Restricted Project, Restricted Project
erichkeane added a comment to D125802: Fix std::has_unique_object_representations for _BitInt types with padding bits.

I think I agree with the justification here, though am a touch confused in the test. I'm also a touch concerned that we have getSubobjectSizeInBits returning a 'rounded up to power of 2' bit happening here. The bitfield case returns non-powers-of-two, but the _BitInt case does not.

May 17 2022, 9:26 AM · Restricted Project, Restricted Project
erichkeane committed rG2def74bef15a: Fix release note typo from 6da3d66f (authored by erichkeane).
Fix release note typo from 6da3d66f
May 17 2022, 6:36 AM · Restricted Project, Restricted Project
erichkeane committed rG6da3d66f03f9: [concepts] Implement dcl.decl.general p4: No constraints on non-template funcs (authored by erichkeane).
[concepts] Implement dcl.decl.general p4: No constraints on non-template funcs
May 17 2022, 6:22 AM · Restricted Project, Restricted Project
erichkeane closed D125711: [concepts] Implement dcl.decl.general p4: No constraints on non-template funcs.
May 17 2022, 6:22 AM · Restricted Project, Restricted Project
erichkeane added a comment to D125711: [concepts] Implement dcl.decl.general p4: No constraints on non-template funcs.

Code and added/modified tests LGTM!

Do you think we should add a release note, given that it could break existing code? Its seems a bit unlikely, but the amount of broken tests have made me a bit worried.
Also, maybe mention the github issue (https://github.com/llvm/llvm-project/issues/51173) in the commit message.

May 17 2022, 5:51 AM · Restricted Project, Restricted Project

May 16 2022

erichkeane added a comment to D124726: Suggest typoed directives in preprocessor conditionals.

@nathanchance

Oops, thank you for your comment!
I would like to work on a hotfix patch for this issue.

@aaron.ballman

Should we entirely opt-out of this suggestion on .S files? I think there are other approaches here, such as decreasing the max edit distance to avoid suggesting this case in .S files, but this approach is avoiding just this edge case so that it would possibly bring a new issue like this. Conversely, opting out of this suggestion on the whole .S files will not be able to catch any typoes. Considering possible edge cases (# directives are also treated as comments), I think opting out would be the only way, unfortunately. What do you think?

For now, I will create a patch opting out of this suggestion on .S files.

May 16 2022, 12:52 PM · Restricted Project, Restricted Project, Restricted Project
erichkeane requested review of D125711: [concepts] Implement dcl.decl.general p4: No constraints on non-template funcs.
May 16 2022, 10:44 AM · Restricted Project, Restricted Project
erichkeane accepted D125517: [Frontend] [Coroutines] Emit error when we found incompatible allocation function in promise_type.
May 16 2022, 6:03 AM · Restricted Project, Restricted Project

May 13 2022

erichkeane added inline comments to D125517: [Frontend] [Coroutines] Emit error when we found incompatible allocation function in promise_type.
May 13 2022, 6:58 AM · Restricted Project, Restricted Project