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.

In D82562#2191512, @dmajor wrote:

@erichkeane, could you help me understand what is the action item of these warnings?

In Firefox we don't require AVX so our compilations generally don't enable the feature. (A very small number of files do come with AVX versions, mostly in imported media codecs, but they are reasonably well-isolated from other code.) It's not clear to me what the warning wants me to do. I can't enable AVX across the board, and I can't change all the code that uses large types as parameters. I feel like the only thing to do is silence the warning since there are enough instances that people will complain about log spam. Have I misunderstood the situation?

Thanks! I'm done for the day, so I'll push in the morning when I'll have a few hours to respond to build bots.

In D84049#2182370, @jfb wrote:

In D84049#2182310, @erichkeane wrote:

In D84049#2182011, @jfb wrote:

In D84049#2181995, @jtmott-intel wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

IF we are going as far as modifying libc++, perhaps we can just add a SFINAE test to std::atomic to disallow the _ExtInt types? That said, it won't help with libstdc++.

Either way, I suspect that is better placed in a different patch, and should no longer hold up this patch.

_Atomic and std::atomic ought to be compatible, they were designed to be. The original point I made was: "_ExtInt doesn't seem to be designed to work with atomic, please do something about it". It seems like disabling it for _Atomic is what you'd want at the moment, until a design for the entire thing is created?

We can ping @jwakely for libstdc++ 😃

In D84049#2182011, @jfb wrote:

In D84049#2181995, @jtmott-intel wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

This seems like a strict improvement as well. That said, there are two opportunities to clean up the code that I suggest evaluating.

I looked through the code changes, and they all seem quite mechanical. I believe they are all correct.

Based on richard's suggestions, that seems about right. However I believe all the asserts should have a message to go along with them as we typicaly do.

Ping! I believe I've done all of the requested changes.

I wasn't able to add you to the bug report, but I discovered this: https://bugs.llvm.org/show_bug.cgi?id=46837

Fix all the things that @rsmith suggested.

Thanks for the review! I'll get this updated in the morning. I DO have a question on your suggestion for the ToVisit/Visited example, so if you could explain a little better, I'd be grateful.

Fix the comment as I mentioned, otherwise this LGTM.

As CWG seems unmoved by my argument as to why CWG2303 should change, I've opted to implement it as worded. As you can see, the implementation is somewhat more complicated, but I believe I made it at least reasonably understandable.

Ive got a bit a of a problem here: This flag (suggest-override) on GCC 8.3 (and others) warns on missing-override EVEN WHEN the function is marked 'final'. This was not fixed until GCC 9.2: https://godbolt.org/z/55KeM3

I'd attempted to regenerate the cxx_dr_status.html using make_cxx_dr_status, but I get a 'no such

Revert change to cxx_dr_status and update the test to have the right tag.

In D84048#2165026, @erichkeane wrote:

In D84048#2165023, @riccibruno wrote:

Also make sure to update the the cxx_dr_status.html document.

It is auto-generated. You need to tag the test case: dr2303: 12.

Edit: And put it at the right place in the file (not at the end).

Do you have an example of this? I've not seen that anywhere before.

In D84048#2165023, @riccibruno wrote:

Also make sure to update the the cxx_dr_status.html document.

It is auto-generated. You need to tag the test case: dr2303: 12.

Edit: And put it at the right place in the file (not at the end).

Also make sure to update the the cxx_dr_status.html document.

Woops, left in a TODO from when I was planning on how to do this patch. Should be ready now :)

I added @rsmith directly, since I suspect he's the most knowledgeable in SemaTemplateDeduction.cpp, but if anyone knows of someone else who is a good fit for this review, please add them as well!

This patch doesn't seem to build for me:
/iusers/ekeane1/workspaces/llvm-project/llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp: In function ‘llvm::Error llvm::exegesis::parseDataBuffer(const char*, size_t, const void*, const void*, llvm::SmallVector<long int, 4>*)’:
/iusers/ekeane1/workspaces/llvm-project/llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:99:37: error: ‘struct perf_branch_entry’ has no member named ‘cycles’

CycleArray->push_back(Entry.cycles);

In D83340#2149100, @jtmott-intel wrote:

@jfb Ah, I feel I better understand your original question now. Thinking through how we would want __atomics to behave is great, and I'm genuinely glad someone's championing it. It also sounds a *smidgen* beyond the scope of what I was trying to accomplish with this particular patch. :-) This patch doesn't alter the behavior of the __atomic builtins one way or the other. It alters the behavior of the __sync builtins only to change a debug assert failure into a diagnostic error. I believe the _ExtInt authors are tracking new bugs related to that feature. A new bug report could keep the discussion going and keep the issue visible.

In D83340#2149100, @jtmott-intel wrote:

@jfb Ah, I feel I better understand your original question now. Thinking through how we would want __atomics to behave is great, and I'm genuinely glad someone's championing it. It also sounds a *smidgen* beyond the scope of what I was trying to accomplish with this particular patch. :-) This patch doesn't alter the behavior of the __atomic builtins one way or the other. It alters the behavior of the __sync builtins only to change a debug assert failure into a diagnostic error. I believe the _ExtInt authors are tracking new bugs related to that feature. A new bug report could keep the discussion going and keep the issue visible.

In D83340#2148615, @jtmott-intel wrote:

@erichkeane Assuming we're back to talking about the __sync functions, I get the following diagnostic.

test.c:22:6: error: address argument to atomic builtin must be a pointer to 1,2,4,8 or 16 byte type ('dtype *' (aka 'unsigned _ExtInt(512) *') invalid)
{    __sync_nand_and_fetch(pval1, val2); }
     ^                     ~~~~~
1 error generated.

Is this what you're referring to? (In highsight I probably could have piggybacked on this diagnostic for the _ExtInt check.)

What about powers-of-2 larger than, say, 256 bit? So a 512 bit value. Does this give reasonable codegen?

Please fix the clang-format concerns, otherwise LGTM.

Gah, misspelled differential revision.

I just took a look at SemaChecking, the rest I'm not sure I get the intent of the patch sufficiently to understand.

As @craig.topper suggested, extract the four lookups into 2 bool instead.

Do all feedback as @craig.topper recommended.

A bunch of clang-format, plus 1 odd looking equation, otherwise this like good.

A few small nits, but I don't sufficiently care enough about the rule, so long as there IS one (hence my reason for opening this can of worms). I added @lattner since he requested that he be added to changes to the coding standards in the past.

I talked to others about the test, and they suggested the correct way to validate this is to make it simply a 'doesn't crash' test, so I'll remove the filecheck/check lines and submit this. The comment there seems sufficient to describe, so I'll live it in place.

1 nit, plus the clang-tidy suggestions.

1 suggestion to the test, but otherwise looks OK. Let me know if you'd like this committed for you.

@LukeZhuang : This patch causes the buildbots to fail, as O1 means something slightly different with the new pass manager :
http://lab.llvm.org:8011/builders/clang-x86_64-debian-new-pass-manager-fast/builds/10542/steps/test-check-all/logs/FAIL%3A%20Clang%3A%3Abuiltin-expect-with-probability.cpp

In D79830#2107404, @LukeZhuang wrote:

Thank you very much!

I'll get this fixed, thanks both of you for letting me know.