Yep, that works, still approved.

Two small changes to the test that you can do as a part of committing. Thanks!

This seems to do a lot of work that I think already exists, I believe I've seen that SPIRV targets already prohibit long-double, so doing another implementation specifically for x86 seems like the wrong approach. I'd suggest seeing if the SPIRV version works for these. If it doesn't, I'd prefer you generalize that implementation.

Just a couple of nits here, basically see how much we can put in the 'cheap to check' branch.

Ah! Thats what I missed!

The patch looks fine to me, though I don't particularly get the test changes that move a diagnostic by 1 line?

Apply clang-format patch, except for the changes to IdentifierTable.

Ok, this should get me up to date! Fixed a bunch of the comments referring to 'expression', added LangOpts::isSYCL, and changed it to DiscriminatorOverride.

Woops! Last update was JUST the changes, and I forgot to squash. Here is the whole patch.

Replace the DeviceLambdaManglingNumber mechanism with the callback mechanism.

Remove 'expression' form per suggestion. Still need to do the device-mangling-number removal/rewrite business.

Fix comments from aaron.

I like this, since the entire CFE does a ton of recursion on purpose for all AST related things. Please give others time to disagree however :)

Do all the things requested by @aaron.ballman and @rjmccall except for limiting the syntax to 'types only'. Currently evaluating whether this is something we can accept.

Apply the clang-format patch from CI, also apply the unused variable fix clang-tidy suggested. The rest of the issues in clang-tidy are identifying functions in a recursive call-chain, but they are all part of the various visitors, so I don't think I have a way to fix those.

No real feedback, however I note that the bug link you have is one that was resolved in 2009, so I suspect it is completely unrelated.

Moved IfStmt/discarded case logic to ExprEvaluatorBase per @rsmith 's suggestion. This is the 'highest' in the tree that has a Sema/ASTContext reference, so anything 'higher' in the tree would be higher touch. @rsmith: WDYT?

In D102261#2751832, @tschuett wrote:

The Ver in Ver2020 is redundant ::2020 would suffice.

In D102261#2751819, @tschuett wrote:

But there is no SYCLMinorVersion today. So enum class SYCLVersion would suffice.

LGTM, but want others to look first.

In D102251#2751101, @ABataev wrote:

LG

I have one 'nit of preference', otherwise I think I don't want to +1 this without giving people a few days to take a look. Based on my looks through the surrounding code, this _LOOKS_ right enough as far as I can tell, but I still want to give others a few days.

I guess my point is: a better comment would have saved me some time. Basically point out that the 'debug' info for the whole type is emitted with a virtual method, and that non-virtual types have it emitted in every TU. Also that this causes it to be emitted in only 1 place, since now there is only a single virtual method definition in a single TU.

Alright, validating it now, then I'll push.

Hrmph... Phab ate my other comment, which was that the name EmitCastBetweenScalarTypes feels clunky. Does EmitScalarCast or EmitScalarScalarCast sound better and capture the meaning correctly?

SGTM.

It is really sad that the attributes can't 'auto transform' themselves. ParsedAttr could (since it has a union of expressions/identifiers), but we don't really seem to have a good way to do it for Attr.

In D73967#2653246, @keryell wrote:

In D73967#2653044, @erichkeane wrote:

In D73967#2653042, @keryell wrote:

Why do you not allow a type able to represent {-1, 0}?

Our FPGA team didn't see any value in it, so we didn't propose it to WG14, so we didn't implement it here. I'm sure a follow-up paper to WG14 would be considered, and if it shows promise/positive reception, we'd welcome a patch here.

It comes from an FPGA tool programmer who knows about HLS *and* modern C++, so probably a very rare specie. :-)
The typical FPGA programmer and tool writer is often more focused on C, so knows barely nothing about generic programming.
In the meantime we can probably use C++20 concepts, a C++ wrapping type and some specialization for the 1bit case to handle this negative cases or just have a SYCL-specific fork...
Probably simpler to have a fork than trying to convince the C committee or FPGA people not understanding C++.

In D73967#2653042, @keryell wrote:

An FPGA programmer is hitting this issue from your unit test:

c++
  signed _ExtInt(1) m; // expected-error{{signed _ExtInt must have a bit size of at least 2}}

Why do you not allow a type able to represent {-1, 0}?

So I guess I don't understand what this does differently than stripPointerCastsAndAliases? Is limiting this to local aliases that important?

The description of the problem, and hte patch and tests seem to make sense, so I think this should be ok. Please give some of the others a little bit of time before committing to speak their final bit, but LGTM .

In D97324#2583286, @jyknight wrote:

I can imagine there being some cases where these could theoretically be useful.

But if you've tested this change and it doesn't cause build failures with any existing uses of TrailingObjects in llvm-project, then LGTM.

For target-specific builtins I ended up implementing this: https://github.com/llvm/llvm-project/commit/81a73fde5cea304d31294fd26c2f051f1685e97c at one point to make sure that 'host' builtins get looked up in the aux-target if necessary.

Can you show any tests where this builtin would be a part of the aux-triple, and not in active code? So an nvptx triple, x86_64 aux-triple, and in non-device code? (such as in a CUDA app)?

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.