Fix looks fine, I'm on the fence about how to handle the test, whether it is valuable to convert it to a C++ test and omit the #ifdef/extra run line, or leave it as it is (without the 2nd run with Wmost).

In D92715#2446479, @craig.topper wrote:

I think it would be possible to add a new attribute to define these types. But only specific values for parameters would be allowed so that it could only generate the exact types we see here and the future segment load patches. It wouldn't be general purpose like vector_size since the backend design has a contract about how to translate the LMUL value from the scalable type.

This would introduce a new type in the AST or at least new subtype of VectorType. This new type would need to be checked for in multiple places in the compiler to define its behavior. As a datapoint VectorType::SveFixedLengthDataVector appears 20 times. It's not clear that's a reduction in complexity versus following what was already done with AArch64SVEACLETypes.def. But I'm not a frontend expert.

In D94092#2481857, @aaron.ballman wrote:

In D94092#2481276, @rjmccall wrote:

Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.

I don't think it will be quite that trivial (switching all identifiers to be parsed as expressions instead). For instance, attributes that take enumeration arguments can parse those arguments as identifiers, but those identifiers will never be found by usual expression lookup because they don't really exist. That said, any attribute that currently accepts an identifier because it really wants to use that identifier in an expression in Sema should update the attribute argument clauses in Attr.td and make the corresponding changes in SemaDeclAttr.cpp/SemaStmt.cpp/SemaType.cpp as appropriate.

In D94092#2479765, @fhahn wrote:

In D94092#2479753, @erichkeane wrote:

I tried a few different things to construct matrix_type attributes with ArgIdents, but failed. The patch also adjusts the code for a bunch of attributes. So if there are indeed cases where ArgIdents can show up, we will get some examples for unit tests.

The comment claims: "// Special case where the argument is a template id.". I would expect one of the following to hit that:
https://godbolt.org/z/znYW1s

I tried this snippet with the patch (and also an added assert(!Attr.isArgIdent(0)); to HandleVectorSizeAttr just to be sure). Compiled as expected, no crash.

In D94092#2479733, @fhahn wrote:

In D94092#2479684, @erichkeane wrote:

I'm not sure how well Attr.td's constraints are enforced on type attributes, as these often happen before parsing is completely done. I'd imagine this code was put into place at least the 1st time for good reason, but I'm curious as to why we wouldn't have tests that cover that (or, as you assert, it could simply be that this is simply dead code).

I'm generally OK with this (the asserts are unnecessary), but would like @aaron.ballman to double check my expectations here.

It would be great if we could get confirmation!

I tried a few different things to construct matrix_type attributes with ArgIdents, but failed. The patch also adjusts the code for a bunch of attributes. So if there are indeed cases where ArgIdents can show up, we will get some examples for unit tests.

I'm not sure how well Attr.td's constraints are enforced on type attributes, as these often happen before parsing is completely done. I'd imagine this code was put into place at least the 1st time for good reason, but I'm curious as to why we wouldn't have tests that cover that (or, as you assert, it could simply be that this is simply dead code).

Ping!

Ping!

In D93597#2477890, @tschuett wrote:

But LLVM is C++14:
https://github.com/llvm/llvm-project/blob/main/llvm/docs/CodingStandards.rst#id13

I note here: https://llvm.org/doxygen/namespacellvm_1_1Intrinsic.html#a7157f9fa9dd11f234ec3c58517cb6d96 that the documentatoin to the getName function variant says:

Since there seems to be at least a little renewed interest in this from a handful of people lately, I spend the time to rebase this and do some minor cleanup tasks.

From the CFE's perspective, this is all that should be needed. Is there a BPF code-owner who can test it and see if it is acceptable?

From my perspective I think this is right... I don't feel comfortable approving this however until someone from OpenCL/OpenMP takes a look.

In D51650#2418749, @lebedev.ri wrote:

In D51650#1305509, @erichkeane wrote:

Fix @rsmith s comments, rebase on the big CPUDispatch refactor.

Ping. What's the status here?

Ping! This was requested in post-commit on the last patch, so I'm hoping it shouldn't be too controversial.

I went through all the comments here, plus looked at the code myself. I believe all of the comments by other reviewers have been fixed/answered acceptably. I don't have any additional comments to add, therefore I think it is appropriate to accept this revision.

Added test for +lambda as @rsmith requested.

In D90634#2371436, @aaron.ballman wrote:

This LGTM but you should wait a day or so in case @rjmccall has opinions.

Made another attempt at fixing the comment. Wordsmithing welcomed/encouraged :)

update comment as @aaron.ballman requested.

@rjmccall : Think I got everything, is this what you mean?

Ping!

Please make sure the commit message is significantly more descriptive here than what is on the review.

Alright, I have the multi-emit dealt with, and the problems that come with that solved. This doesn't do the MSVC-emit-for-all-CCs thing, but I intend to do that in a separate patch with the same infrastructure.

Turns out this patch: https://github.com/llvm/llvm-project/commit/2e1e0491b7098fcfe01945e8f62cafe1fcb3cf36 is my problem. The issue has to do deducing a 'local' type in the return type. As a result, we choose to mangle any type 'containing' auto as 'auto'.

In D88645#2347725, @Tyker wrote:

In D88645#2347050, @aaron.ballman wrote:

LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?

In D89559#2347642, @rjmccall wrote:

Mangling more calling conventions when mangling function types in Itanium (except at the top level) is the right thing to do. There's already a place to do so in the mangling. We just haven't done this yet because a lot of those calling convention are supported by other compilers, so we need to coordinate. So you need to check out what other compilers do (GCC, ICC) and imitate them if they're already setting a reasonable default; otherwise, I think there's a standard algorithm for generating these.

Separately, the MSVC mangler should support Clang's CCs if there's a reasonable extensible rule there. I've never been able to figure out the MVSC mangling grammar.

Same here :)

Hmm... so I got distracted the last few days with a handful of small SEMA issues that I believe to be solved, so I'm going back to my codegen issues.

In D89559#2340357, @rsmith wrote:

In D89559#2339206, @erichkeane wrote:

Since the work is 99% the same, I think I should start doing that (emitting BOTH in the case where it matches the default member but NOT the default free function CC) in this review, then do a separate commit for the MSVC compat mode where we emit a version for ALL of the calling conventions.

You'll also need an overload resolution tiebreaker to ensure that +[]{} and the like are not ill-formed due to ambiguity.

In D89559#2339192, @rjmccall wrote:

Yeah, it might be reasonable to just do that on targets where the defaults are different (just MSVC, I think?) and otherwise preserve the method's calling convention.

In D86936#2338664, @erichkeane wrote:

Why did we select the 'bracket-depth' for this? The documentation on that doesn't seem to be anything close to what this should be based on:

Sets the limit for nested parentheses, brackets, and braces to N in blocks, declarators, expressions, and struct or union declarations.

The 256 default limit is REALLY small for a fold expression, particularly since the instantiation depth limit is 1024 by default. I think this patch needs to be changed to use the InstantiationDepth instead. @rjmccall, thoughts?

Why did we select the 'bracket-depth' for this? The documentation on that doesn't seem to be anything close to what this should be based on:

In D89559#2336335, @rjmccall wrote:

We can't have it always match, that would require us to reject the conversion to a bare function-pointer type, which is required by the standard.

I'm not sure if MSVC's multiple conversions hack is conformant or not, but it's at least more conformant than that.

Perhaps a better example:

In D89559#2335557, @rjmccall wrote:

No, if you put a calling convention on a lambda and then convert it to a function pointer, you almost certainly want that CC to be honored.

Is the AttributedType still present in CallOperator->getType()?

Calc->get.

In D76620#2328305, @rjmccall wrote:

In D76620#2328031, @erichkeane wrote:

In D76620#2328011, @rjmccall wrote:

In D76620#2327988, @erichkeane wrote:

In D76620#2327976, @rjmccall wrote:

In D76620#2327910, @erichkeane wrote:

In D76620#2327901, @rjmccall wrote:

You know on both sides that a lambda is used as a kernel, yes? Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence?

Coming up with said stable sequence is somewhat difficult as well. A strict integer ordering didn't end up being stable, as some of the kernel handling can cause instantiations to happen in a slightly different ordering, which messed that up, as would use of the builtin. We wanted to find a way that was dependent on the source code alone. The "Quick and Dirty" solution was line/column numbers, though we considered a hash of that same information to at least make it shorter.

Certainly you wouldn't want a *global* sequence ID. However, lambdas can't just occur globally, they're always part of some declaration that they can be scoped to, so that you have e.g. "the third kernel lambda within function X". I fail to see how that wouldn't address the concern about instantiation order, and it's still source-directed.

Hmm... I'll have to consider that. That is an interesting thought. Presumably we could just copy the 'getLambdaManglingNumber' stuff in that case, and place the value in the same location in the mangling, with some sort of discriminator (to avoid conflicting manglings).

The Itanium mangling already produces a different lambda mangling number for different lambda signatures. You just need the kernel-ness to be part of the signature.

It does it via the getLambdaManglingNumber I believe, right? The idea would be to have a getKernelLambdaManglingNumber that does something similar, and needs something to avoid conflicting, which is the discriminator I was mentioning.

Right, so if you look how that's ultimately derived in ItaniumNumberingContext::getManglingNumber, you'll see that every context has different sequences for unique lambda-sigs. If you can just put "this is a kernel" into the lambda-sig, you'll automatically get a stable sequence for kernel lambdas. But that would rely on immediately knowing that a lambda will be used as a kernel.

The only other concern I have is WHEN we know that a lambda is used in a kernel, which we don't until it is called (and can cause confusion when it is a template parameter and called later).

Oh, yes, if you don't know that a lambda is used as a kernel locally then this falls apart a bit. You could of course pretend for ABI purposes that there's a new intermediate lambda at the kernel use point when the lambda is not local to the current function. I don't think there's anything which relies on mangling lambdas before the function they're contained within is fully type-checked.

One thing that might save us from this, is we aren't actually modifying the LAMBDAs mangling, we're modifying the KERNEL's mangling. The lambda itself keeps its mangling, but the kernel that calls it (which is the device/host boundary) is what has its name changed. So at least at that point we know some about it. And, actually, since we are generating the 'integeration header' lookup table AND the kernel in identical (if not the same) invocation, perhaps a global counter WOULD work... My coworker is investigating this now, so hopefully she'll be able to prove out this idea and give further feedback here.

I feel obliged to point out that, if you're worried about a single function potentially having a different set of lambdas in different translation modes, that's also going to affect things like template specialization identity. That is, suppose a kernel function is formed for a particular lambda, and in one translation mode it's the first lambda with that signature in a function and in another mode it's the second. That's going to change the immediate mangling of the lambda, but it's also going to change the mangling of templates used with that lambda. For example, if you have

In D76620#2328011, @rjmccall wrote:

In D76620#2327988, @erichkeane wrote:

In D76620#2327976, @rjmccall wrote:

In D76620#2327910, @erichkeane wrote:

In D76620#2327901, @rjmccall wrote:

You know on both sides that a lambda is used as a kernel, yes? Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence?

Coming up with said stable sequence is somewhat difficult as well. A strict integer ordering didn't end up being stable, as some of the kernel handling can cause instantiations to happen in a slightly different ordering, which messed that up, as would use of the builtin. We wanted to find a way that was dependent on the source code alone. The "Quick and Dirty" solution was line/column numbers, though we considered a hash of that same information to at least make it shorter.

Certainly you wouldn't want a *global* sequence ID. However, lambdas can't just occur globally, they're always part of some declaration that they can be scoped to, so that you have e.g. "the third kernel lambda within function X". I fail to see how that wouldn't address the concern about instantiation order, and it's still source-directed.

Hmm... I'll have to consider that. That is an interesting thought. Presumably we could just copy the 'getLambdaManglingNumber' stuff in that case, and place the value in the same location in the mangling, with some sort of discriminator (to avoid conflicting manglings).

The Itanium mangling already produces a different lambda mangling number for different lambda signatures. You just need the kernel-ness to be part of the signature.

In D76620#2327976, @rjmccall wrote:

In D76620#2327910, @erichkeane wrote:

In D76620#2327901, @rjmccall wrote:

You know on both sides that a lambda is used as a kernel, yes? Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence?

Coming up with said stable sequence is somewhat difficult as well. A strict integer ordering didn't end up being stable, as some of the kernel handling can cause instantiations to happen in a slightly different ordering, which messed that up, as would use of the builtin. We wanted to find a way that was dependent on the source code alone. The "Quick and Dirty" solution was line/column numbers, though we considered a hash of that same information to at least make it shorter.

Certainly you wouldn't want a *global* sequence ID. However, lambdas can't just occur globally, they're always part of some declaration that they can be scoped to, so that you have e.g. "the third kernel lambda within function X". I fail to see how that wouldn't address the concern about instantiation order, and it's still source-directed.

In D76620#2327901, @rjmccall wrote:

You know on both sides that a lambda is used as a kernel, yes? Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence?

In D76620#2327617, @yaxunl wrote:

CUDA/HIP are facing similar issues, i.e. consistency of name mangling of kernels between host/device compilation of the same TU. I hope this feature to be implemented in a generic way so that it may be reusable for other offloading languages.

In D76620#2326551, @rsmith wrote:

In D76620#2324858, @erichkeane wrote:

The feature that this supports is a part of the SYCL 2020 Provisional Spec, I thought that was sufficient. We'll look into an RFC to re-submit in the future.

Does that cover only functionality that can be implemented in terms of this builtin, or the builtin itself? If so, what is the functionality that needs to be supported? That information will be a good starting point for the RFC.

In D76620#2324119, @rjmccall wrote:

Richard and I just found out about this commit, and we've decided to revert it. I apologize for the very late reversion, but the reality of Clang development is that it's very difficult for the code owners to watch literally every commit, and we rely on the community to follow policies and bring things to our attention when there's a question about the right thing to do. This is a new feature that was submitted without properly following any of the guidelines for new features: it was neither approved by a relevant standards body, added for compatibility with an existing frontend, or taken through the RFC process. If you had brought it to our attention as an RFC, we would've expressed serious concerns about the design.

Please start an RFC thread and CC Richard and me.

*PING!* Any feedback?

I'm not really a good person to review this, OMP isn't nearly my expertise.

Ok then, I'll take a look to see what it would take to make the MaterializeTemporaryExpr be created with the complete type here instead. Thanks!

Gah, forgot 'Differential Revision' again :/

@rjmccall Done!

Note: @rsmith and @rjmccall were the last ones in this section based on Blame (by a wide margin!), so added both as reviewers.

This looks right to me.

Its a bit of a shame that the two implementations (left/right) differ by only a single character, an y ideas for combining them cleanly? I might also consider combining the two 'if' statements into a single one (if !EvalInteger(E->getArg(0)...) || !EvalInteger(E->getArg(1)...), but that is generally just preference.

CFE changes look right to me. Please give the rest of the reviews a little while to take a look though

Were you able to make any progress on this?

1 comment, otherwise seems alright to me.

Yep! Declaring a global variable that isn't 'extern' with an incomplete type is disallowed anyway, so if you call RequireCompleteType, you're likely just diagnosing that early.

I did a little debugging, and the problem is the template itself isn't a complete type until you require it with RequireCompleteType. You have this same problem with incomplete types: https://godbolt.org/z/hvf3M4

Also, see this bug: This crashes immediately when used on a template instantiation: https://bugs.llvm.org/show_bug.cgi?id=47169

In D74361#1927931, @JonChesterfield wrote:

In D74361#1927863, @thakis wrote:

This breaks tests on Windows: http://45.33.8.238/win/10664/step_7.txt

Please take a look, and if it takes some time please revert while you investigate.

Thanks! It seems Windows inserts 'dso_local' into the middle of the generated IR.

I can't test on Windows so the two that failed CI are now marked as "UNSUPPORTED: system-windows".

Do you know a usual work around for variation in symbol visibility? I'm happy to copy it from another test but am wary of guessing what might work on Windows.