In D63507#1552849, @sdesmalen wrote:

In D63507#1551968, @erichkeane wrote:

Can someone take a look at this?
@sdesmalen in particular, I'd love it if you could take a look since you're the author of this code.

Thanks, I definitely want to take a look but I’m currently traveling without access to my laptop, so I won’t be able to properly review it until Wednesday.

Can someone take a look at this?
@sdesmalen in particular, I'd love it if you could take a look since you're the author of this code.

Update the comment as requested by @rjmccall

In D56571#1523895, @nickdesaulniers wrote:

Was this committed accidentally?

Today in master I see:

• r362106: Revert "clang support gnu asm goto." Erich Keane <erich.keane@intel.com>
• r362059: Mark CodeGen/asm-goto.c as x86 specific after r362045 Fangrui Song <maskray@google.com>
• r362045: clang support gnu asm goto. Jennifer Yu <jennifer.yu@intel.com>
  
  and yet this Phabricator review is still open.
  
  It may be easier to rebase this into a monorepo checkout, then commit with git llvm push. https://llvm.org/docs/GettingStarted.html#for-developers-to-commit-changes-from-git
  
  As @kees noted, for new test files that contain x86 assembly, they need a -triple that specifies an x86 target, otherwise the implicit target is based on the host, which may not be x86. We still want to verify that non-x86 host can cross compile to x86 correctly. Sorry we did not catch this earlier in code review.

In D62435#1524824, @mstorsjo wrote:

This change broke compiling Qt on MinGW, see https://bugs.llvm.org/show_bug.cgi?id=42089. Trivially reproducible by trying to compile a snippet that looks like this:

class Foo {
public:
        __attribute__((nothrow)) void __attribute__((__stdcall__)) Bar();
};

void Foo::Bar() {
}

Reproducible by compiling the same snippet for a windows-msvc target as well.

Conditional approval, commit with the comment fixed. Let me know if you need me to commit it for you.

A different (perhaps silly) question is why 'W' isn't sufficient? It represents int64_t, which I wonder if is sufficient.

'O' is an interesting choice. Any real justification for it, or just "what was available"? It definitely needs to be documented in the top of Builtins.def however.

Added warning + other comments from @aaron.ballman

Reread @rsmith's comments and surrounding code and found what I believe is the correct answer to these comments :)

Woops! Look like I left without actually changing the revision *shame*.

As @rnk suggested, switch to an enum. SemaType.cpp unfortunately has Sema as a forward declaration, so it has to use integers.

@mgorny @hfinkel @eli.friedman @jyknight @dim
Is there any chance we can get this in any time soon? It fixes a couple of header issues that we've noticed. Others added, since it appears this dependent on https://reviews.llvm.org/D29542 ?

This is my concern here:
https://godbolt.org/z/icS4fa
The patch will change template instantiation.

Thank you @rsmith for the quick review! I really appreciate it.