[PATCH] D93922: Mangle `__alignof__` differently than `alignof`.

jyknight updated this revision to Diff 318277. jyknight marked 8 inline comments as done. jyknight added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93922/new/ https://reviews.llvm.org/D93922 Files: clang/lib/AST/I

[PATCH] D93922: Mangle `__alignof__` differently than `alignof`.

jyknight added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:4010 +if (Context.getASTContext().getLangOpts().getClangABICompat() >= +LangOptions::ClangABI::Ver11) { + Out << "u8__uuidof"; rsmith wrote: > Should this be `>= Ver12` /

[PATCH] D93922: Itanium Mangling: Mangle `__alignof__` differently than `alignof`.

jyknight updated this revision to Diff 319422. jyknight retitled this revision from "Mangle `__alignof__` differently than `alignof`." to "Itanium Mangling: Mangle `__alignof__` differently than `alignof`.". jyknight added a comment. Minor updates. Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D95487: Itanium Mangling: Fix handling of in .

jyknight created this revision. Herald added a subscriber: kristof.beyls. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Previously, we were emitting an extraneous X .. E in around an if the template argument was constructed fro

[PATCH] D95488: Itanium Mangling: In 'enable_if', omit X/E around .

jyknight created this revision. jyknight added reviewers: rsmith, rjmccall. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The Clang enable_if extension is mangled as an , which is supposed to contain . However, we were unconditio

[PATCH] D93922: Itanium Mangling: Mangle `__alignof__` differently than `alignof`.

jyknight added a comment. OK, I've posted two follow-up changes now. https://reviews.llvm.org/D95487 fixes the issue with mangleTemplateArg given expressions -- this turned out to be a larged-scoped problem than I had thought, affecting manglings other than just that used the matrix extension.

[PATCH] D95487: Itanium Mangling: Fix handling of in .

jyknight added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:3912 +IsPrimaryExpr = false; + }; + rjmccall wrote: > I think it might be more reasonable to just check for the relatively small > number of primary-expression cases in `mangleTempl

[PATCH] D95488: Itanium Mangling: In 'enable_if', omit X/E around .

jyknight updated this revision to Diff 319578. jyknight added a comment. Add neglected clang-abi-compat.cpp change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95488/new/ https://reviews.llvm.org/D95488 Files: clang/lib/AST/ItaniumMangle.cpp

[PATCH] D95487: Itanium Mangling: Fix handling of in .

jyknight added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:3912 +IsPrimaryExpr = false; + }; + jyknight wrote: > rjmccall wrote: > > I think it might be more reasonable to just check for the relatively small > > number of primary-expression

[PATCH] D95487: Itanium Mangling: Fix handling of in .

jyknight added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:5145 + ASTContext &Ctx = Context.getASTContext(); + if (Ctx.getLangOpts().getClangABICompat() > LangOptions::ClangABI::Ver11) { +mangleExpression(E, UnknownArity, /*AsTemplateArg=*/true); --

[PATCH] D93922: Itanium Mangling: Mangle `__alignof__` differently than `alignof`.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9c7aeaebb3ac: Itanium Mangling: Mangle `__alignof__` differently than `alignof`. (authored by jyknight). Repository: rG LLVM Github Monorepo CHAN

[PATCH] D95487: Itanium Mangling: Fix handling of in .

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8ca33605ff0c: Itanium Mangling: Fix handling of in . (authored by jyknight). Changed prior to commit: https://reviews

[PATCH] D95488: Itanium Mangling: In 'enable_if', omit X/E around .

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa7246ba02a89: Itanium Mangling: In 'enable_if', omit X/E around . (authored by jyknight). Repository: rG LLVM Github Monorepo CHANG

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

jyknight added a comment. I still have the same fundamental objection as before to the parts of this patch for prohibiting FP add/sub on some targets. If a particular LLVM target cannot handle transforming an FP add/sub (or any other RMW operations!) into the correct cmpxchg or LL/SC loop, that

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

jyknight added a comment. My concern is that this is treating a backend _bug_ as if it were just an optional feature. But it's not the case that it might be reasonable to either implement or not implement this in a backend -- it should be implemented, and those that don't are buggy. I'd be hap

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

jyknight added a comment. In D71726#2537101 , @yaxunl wrote: > For amdgpu target, we do need diagnose unsupported atomics (not limited to fp > atomics) since we do not support libcall due to ISA level linking not > supported. This is something we cannot

[PATCH] D96044: DebugInfo: Emit "LocalToUnit" flag on local member function decls.

jyknight created this revision. jyknight added a reviewer: dblaikie. Herald added a subscriber: inglorion. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Previously, the definition was so-marked, but the declaration was not. This

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.attachedcall' instead of explicitly emitting retainRV or claimRV calls in the IR

jyknight added a comment. > I ended up reverting the changes I made to llvm/lib/IR/AutoUpgrade.cpp as the > file was including llvm/Analysis/ObjCARCUtil.h, which was violating layering. It looks like it was not actually reverted in the version ultimately submitted. I've pushed commit 3c06676de1

[PATCH] D97223: Add Alignment argument to IRBuilder CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight created this revision. jyknight added reviewers: gchatelet, jfb, rjmccall. Herald added subscribers: teijeong, dexonsmith, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, antiagainst, shauheen, rriddle, mehdi_ami

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight created this revision. jyknight added reviewers: gchatelet, jfb, rjmccall. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Following the LLVM change to add an alignment argument to the IRBuilder calls, switch Clang's CGBui

[PATCH] D94213: Clang: Remove support for 3DNow!, both intrinsics and builtins.

jyknight added a comment. Herald added a subscriber: jansvoboda11. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94213/new/ https://reviews.llvm.org/D94213 ___ cfe-commits mailing list cfe-commits@

[PATCH] D94252: Delete (most) of the MMX builtin functions from Clang.

jyknight added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94252/new/ https://reviews.llvm.org/D94252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D86855: Convert __m64 intrinsics to unconditionally use SSE2 instead of MMX instructions.

jyknight added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86855/new/ https://reviews.llvm.org/D86855 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D96044: DebugInfo: Emit "LocalToUnit" flag on local member function decls.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGfe2dcd89acfd: DebugInfo: Emit "LocalToUnit" flag on local member function decls. (authored by jyknight). Repository: rG LLVM Github Monorepo CHAN

[PATCH] D97223: Add Alignment argument to IRBuilder CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight added a comment. In D97223#2581126 , @rjmccall wrote: > Looks like you didn't update the .bc reader/writer and the .ll printer/parser. The instructions already had alignment support added to the type constructor and IR/BC in prior changes by gch

[PATCH] D97324: [NFC] Make TrailingObjects non-copyable/non-movable

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. 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 TrailingObjec

[PATCH] D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when init_array is not used.

jyknight added a comment. Won't this change cause weird effects with LTO, when you're merging multiple TUs' global_ctors arrays before emitting? Won't this end up reversing the order of the files, as well as the order of the functions within a single file? That does not seem likely to be expect

[PATCH] D103987: Start tracking Clang's C implementation status

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. Sounds like a great plan to me. I might suggest adding some text about the page being incomplete, so that people don't wonder if the blank sections for pre-C2x are a bug. Repository: r

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight added a comment. Ping. I think this is correct, and would like to commit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97224/new/ https://reviews.llvm.org/D97224 ___ cfe-commits mailing list cf

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. In D106577#2904960 , @rsmith wrote: >> One specific example I'd like to be considered: >> Suppose the C standard library implementation's mbstowcs converts a certain >> multi-byte character C to somewhere in the Unicode private

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

jyknight added a comment. It appears as though this commit was reverted in Apple's XCode Clang fork -- the behavior currently in XCode matches the behavior of upstream Clang prior to this patch. Presuming that's correct, I think we should revert this upstream as well. There doesn't seem to be a

[PATCH] D108243: Revert "Avoid needlessly copying a block to the heap when a block literal"

jyknight created this revision. jyknight added reviewers: ahatanak, erik.pilkington, rjmccall. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. It has been reverted in Apple's Clang fork for quite some time, possibly ever since it w

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

jyknight added a comment. I've gone ahead and created a revert review: https://reviews.llvm.org/D108243 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58514/new/ https://reviews.llvm.org/D58514 ___ cfe-commits mailing l

[PATCH] D105142: RFC: Implementing new mechanism for hard register operands to inline asm as a constraint.

jyknight added a comment. This code doesn't handle multiple alternatives in a constraint. E.g. `"={eax}{ebx}"` or `"={eax}{ebx},m"`. See the GCC docs for the C-level syntax https://gcc.gnu.org/onlinedocs/gcc/Multi-Alternative.html#Multi-Alternative and LLVM IR docs for the IR syntax: https://llv

[PATCH] D104500: [clang] Apply P1825 as Defect Report from C++11 up to C++20.

jyknight added a comment. This commit seems to have broken libc++ in C++98 mode, as it appears to have depended upon the implicit-move extension. Reproduction is simple. Build this with `-stdlib=libc++ -std=c++98`: #include void foo (std::set *s) { s->insert(5); } (https://godbolt.

[PATCH] D103465: [OpaquePtr] Track pointee types in Clang

jyknight added inline comments. Comment at: clang/lib/CodeGen/Address.h:26 llvm::Value *Pointer; + llvm::Type *PointeeType; CharUnits Alignment; erichkeane wrote: > I think this will still end up being a problem when we try to look into the > type for mul

[PATCH] D112400: [clang][compiler-rt][atomics] Add `__c11_atomic_fetch_nand` builtin and support `__atomic_fetch_nand` libcall

jyknight added inline comments. Comment at: compiler-rt/lib/builtins/atomic.c:339 +#define ATOMIC_RMW_NAND(n, lockfree, type) \ + type __atomic_fetch_nand_##n(type *ptr, type val, int model) { \ Same as ATOMI

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

jyknight created this revision. jyknight added a reviewer: aaron.ballman. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Per C++17 [except.spec], 'throw()' has become equivalent to 'noexcept', and should therefore call std::termin

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

jyknight added a comment. In D113517#3120030 , @rsmith wrote: > What's the motivation for this change? I believe the current behavior is > still conforming: `set_unexpected` is no longer (officially) part of the > standard library (though it still exist

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

jyknight added a comment. In D113517#3122217 , @rsmith wrote: > In D113517#3121455 , @jyknight > wrote: > >> This change allows those future optimizations to apply to throw() as well, >> in C++17 mode, which is

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

This revision was automatically updated to reflect the committed changes. Closed by commit rGfddc4e41164e: Correct handling of the 'throw()' exception specifier in C++17. (authored by jyknight). Changed prior to commit: https://reviews.llvm.org/D113517?vs=385971&id=386326#toc Repository: rG

[PATCH] D97223: Add Alignment argument to IRBuilder CreateAtomicRMW and CreateAtomicCmpXchg.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG24539f1ef247: Add Alignment argument to IRBuilder CreateAtomicRMW and CreateAtomicCmpXchg. (authored by jyknight). Herald added a subscriber: cota.

[PATCH] D97223: Add Alignment argument to IRBuilder CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight added inline comments. Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:1037 + assert(AddrAlign >= ResultTy->getPrimitiveSizeInBits() / 8 && + "Expected at least natural alignment at this point."); jrtc27 wrote: > Doesn't this introduce anoth

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight updated this revision to Diff 328219. jyknight marked 2 inline comments as done. jyknight added a comment. Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97224/new/ https://reviews.llvm.org/D97224 Files: clang/li

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight added a comment. In D97224#2596410 , @rjmccall wrote: > Do we really consider the existing atomic intrinsics to not impose added > alignment restrictions? I'm somewhat concerned that we're going to produce > IR here that's either really subopti

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight added a comment. > The idea here is not to "ignore type alignment". `EmitPointerWithAlignment` > will sometimes return an alignment for a pointer that's less than the > alignment of the pointee type, e.g. because you're taking the address of a > packed struct field. The critical ques

[PATCH] D86855: Convert __m64 intrinsics to unconditionally use SSE2 instead of MMX instructions.

jyknight added a comment. Ping, thanks! Or, if you have suggestions on how to make it easier to review, I'd be open to that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86855/new/ https://reviews.llvm.org/D86855 ___

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight updated this revision to Diff 330088. jyknight added a comment. Use natural alignment for `_Interlocked*` intrinsics. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97224/new/ https://reviews.llvm.org/D97224 Files: clang/lib/CodeGen/CGAt

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

jyknight added a comment. In D97224#2621328 , @rjmccall wrote: > In D97224#2604537 , @jyknight wrote: > >>> I'm less certain about what to do with the `__atomic_*` builtins >> >> The `__atomic` builtins have already

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

jyknight added a comment. I think this patch was wrong -- we should not be assuming 16-byte alignment for an allocation smaller than 16 bytes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879

[PATCH] D136707: [clang][Toolchains][Gnu] pass -gdwarf-* through to assembler

jyknight added a comment. This change has caused `clang -Wall -fdebug-default-version=5 -c -xc /dev/null -o /dev/null` to print the following warning, because it has moved the read of the argument into a conditional. `clang: warning: argument unused during compilation: '-fdebug-default-version=

[PATCH] D129833: Use @llvm.threadlocal.address intrinsic to access TLS

jyknight added a comment. Note that this change caused LLVM to no longer be aware that a TLS variable cannot be NULL. Thus, code like: __thread int x; int main() { int* y = &x; return *y; } built with `clang -O -fsanitize=null` emits a null-pointer check, when it wouldn't previou

[PATCH] D134550: [Clang] Make Clang driver suggest '-Xclang' for CC1 options passed to the driver

jyknight added a comment. I //really// don't think we should have this behavior. The cc1 options are supposed to be an internal implementation detail. It's already a problem that the option name doesn't shout "hey I'm an internal interface with no stability guarantees! Don't use me!". Having cl

[PATCH] D134550: [Clang] Make Clang driver suggest '-Xclang' for CC1 options passed to the driver

jyknight added a comment. In D134550#3813269 , @aaron.ballman wrote: > Alternatively, perhaps those experimental options should be exposed from the > driver instead of being a cc1-only flag? IMO: yes. If we want end-users to use a particular flag, we s

[PATCH] D142925: [Clang] Improve error message for violations of -fmodules-decluse.

jyknight created this revision. jyknight added a reviewer: ChuanqiXu. Herald added a project: All. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Now it reports the name of the indirectly-used module which is missing. Repository

[PATCH] D142925: [Clang] Improve error message for violations of -fmodules-decluse.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGab0116e2f05c: [Clang] Improve error message for violations of -fmodules-decluse. (authored by jyknight). Repository: rG LLVM Github Monorepo CHAN

[PATCH] D127545: Remove Itanium EH ABI workaround to allow modifying a pointer caught by-reference.

jyknight created this revision. jyknight added reviewers: rjmccall, nikic. Herald added a project: All. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Pointers will now always be caught by-value. The workaround was originally add

[PATCH] D127545: Remove Itanium EH ABI workaround to allow modifying a pointer caught by-reference.

jyknight added a comment. (Note: if folks agree with this direction, I'll file a new bug for the newly broken behavior -- even though we probably cannot actually fix it -- and link it from the commit.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D125814: Improve the strict prototype diagnostic behavior

jyknight added a comment. Good improvement, but an additional change to wording for just the zero-arg non-prototype function declaration case could help a lot. The fact that zero-arg it's only being warned about because of the "...because of" note isn't particularly clear -- especially when the

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

jyknight added a comment. In D126324#3536785 , @aaron.ballman wrote: > The changes so far look sensible, but I think we should add some more tests > for a few situations. 1) Using a const weak symbol as a valid initializer > should be diagnosed (with a

[PATCH] D125814: Improve the strict prototype diagnostic behavior

jyknight accepted this revision. jyknight added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaDecl.cpp:3959 + // it's a user-visible declaration. There is one exception to this: + // when the new declaration is

[PATCH] D126170: C++ DR2394: Const-default-constructible for members.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG997b072e10d2: C++ DR2394: Const-default-constructible for members. (authored by jyknight). Changed prior to commit: https://reviews.llvm.org/D1261

[PATCH] D125291: Introduce @llvm.threadlocal.address intrinsic to access TLS variable (1/3)

jyknight added a comment. So, I've been spending some significant time looking into this. I found that I couldn't really review this change for correctness, because I find it basically impossible to figure out whether the intrinsic calls have actually been added to all the right places in Clang

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

jyknight added a comment. In D125919#3556754 , @rsmith wrote: > That assumption does not hold. Given `struct A { char c[3]; };`, `struct A` > has size 3 and align 1, but `_Atomic struct A` has size 4 and align 4 across > many (perhaps all?) of our targe

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

jyknight reopened this revision. jyknight added a comment. This revision is now accepted and ready to land. Reopening the review since it ended up reverted, but I do think we DO want to have at least some of the changes proposed here (and/or from the pending D158372

[PATCH] D156565: Diagnose use of VLAs in C++ by default

jyknight added a comment. Overall looks good, just a few nits from looking things over. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:143 +def ext_vla_cxx : ExtWarn< + "variable length arrays are a Clang extension">, InGroup; +def ext_vla_cxx_in_gnu_mode : Exten

[PATCH] D156565: Diagnose use of VLAs in C++ by default

jyknight added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2617 + } else if (getLangOpts().CPlusPlus) { +if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context)) + VLADiag = getLangOpts().GNUMode aaron.ballman wrote: > jykni

[PATCH] D156565: Diagnose use of VLAs in C++ by default

jyknight added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2617 + } else if (getLangOpts().CPlusPlus) { +if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context)) + VLADiag = getLangOpts().GNUMode aaron.ballman wrote: > jykni

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

jyknight added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { efriedma wrote: > nickdesaulniers wrote: > > efriedma wrote: > > > nickdesaulniers wrote: > > >

[PATCH] D141918: WIP: [Clang] Emit 'unwindabort' when applicable.

jyknight added a comment. In D141918#4566838 , @smeenai wrote: > $ clang -std=c++20 -O2 -c crash.cpp > cannot use musttail with unwindabort Thanks for the report. We were missing a check of `CI.isUnwindAbort()` in CoroSplit.cpp's `shouldBeMustTail()` fu

[PATCH] D157793: [Headers] Add missing __need_ macros to stdarg.h

jyknight added a comment. I'm a bit confused by this change vs its description. It looks like stdarg already supported `__need___va_list`, which is what you said Apple needs. Does Apple //also// require the other __need_va_copy/etc, too? If not, why add support for them? (I think we should pref

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

jyknight added a comment. > The diagnostic behavior is correct. MYTHING doesn't get expanded until phase > 4 (http://eel.is/c++draft/lex.phases#1.4), so this appears as "ONE"MYTHING as > a single preprocessor token: > https://eel.is/c++draft/lex.ext#nt:user-defined-string-literal and that token

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

jyknight added a comment. In D153156#4599595 , @rZhBoYao wrote: > What if a programmer is really trying to call operator""b here (albeit > ill-formed) Because that code is ill-formed, Clang diagnosed it with an error by default. Isn't that preferable t

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

jyknight added a comment. As I posted on the RFC thread, I think we have a viable alternative solution to address the original motiving use-case. One might potentially still make a case for implementing the `-fno-coroutines` flag for GCC compatibility, but given the concerns raised -- and that

[PATCH] D156286: [docs] Bump minimum GCC version to 7.5

jyknight added a comment. Given that folks have successfully tested with GCC 7.4, and the lateness of the change in the release process for LLVM 17, I think it'd be better to require GCC 7.4 (the earliest that actually works), instead of increasing the requirement to 7.5. Repository: rG LLV

[PATCH] D141918: WIP: [Clang] Emit 'unwindabort' when applicable.

jyknight updated this revision to Diff 546613. jyknight edited the summary of this revision. jyknight added a comment. Rebase patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141918/new/ https://reviews.llvm.org/D141918 Files: clang/lib/Code

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

jyknight added a comment. In D155387#4557834 , @hctim wrote: > I found an issue with building Android using this patch. I've reduced it down > to the following problem where the evaluation of the `std::visit` is believed > to be non-exhaustive, but it s

[PATCH] D159250: [X86][RFC] Add new option `-m[no-]evex512` to disable ZMM and 64-bit mask instructions for AVX512 features

jyknight added a comment. In D159250#4633530 , @pengfei wrote: > In D159250#4631786 , @RKSimon wrote: > >> Would it be possible to add function multiversioning tests to ensure the >> evex512 attribute would work

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

jyknight added a comment. I don't think there's a particularly good reason to expose this as a builtin, unless it's to be used by the standard library implementation. (Which again I do not think we should implement.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D69868: Allow "callbr" to return non-void values

jyknight added a comment. Ugh, it's actually been that long, hasn't it...I'm really sorry about that. :( I've been actively spending time to look at this over the last couple weeks. I haven't been able to convince myself that the weird-successors and having allocatable registers across BBs here

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

jyknight added a comment. As I've mentioned before, depending on what this will be used for, "64" is not a _useful_ answer if you want to know how your memory accesses will behave on modern intel x86 CPUs, despite being the "correct" answer for cache-line size. But, modern intel CPUs fetch alig

[PATCH] D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth

jyknight requested changes to this revision. jyknight added a comment. This revision now requires changes to proceed. This isn't correct. The atomic interface is designed to be forward-compatible with new CPUs that have wider atomic instructions. That clang has a MaxAtomicPromoteWidth value _at

[PATCH] D75009: [Diagnostic] Improve discoverability of ftabstop for misleading indentation

jyknight added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:67 +def note_misleading_indentation_tab_space_mix : Note< + "there is a mix of tabs spaces; the tab size (-ftabstop=X) is set to %0">; Maybe "assuming tabstops every

[PATCH] D75056: [Driver] Default to -fno-common for all targets

jyknight added a comment. Are there any tests remaining that check that with -fcommon, IR generation creates "common" variables, now that all these tests have been modified? If there are not, one should be added. Comment at: clang/docs/ClangCommandLineReference.rst:1311 +Plac

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

jyknight requested changes to this revision. jyknight added a comment. This revision now requires changes to proceed. In D74918#1885869 , @zoecarver wrote: > @jyknight It would probably be best if we could account for CPUs who like to > use aligned pairs

[PATCH] D75056: [Driver] Default to -fno-common for all targets

jyknight accepted this revision. jyknight added a comment. Looks good modulo minor comments. Comment at: clang/test/CodeGen/microsoft-no-common-align.c:1 // RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -o - %s | FileCheck %s typedef float TooLargeAlignment __attrib

[PATCH] D69756: [opaque pointer types] Add element type argument to IRBuilder CreatePreserveStructAccessIndex and CreatePreserveArrayAccessIndex

jyknight added a comment. Updated release notes in d11a9018b773c0359934a7989d886b02468112e4 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69756/new/ https://reviews.llvm.org/D6

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

jyknight added a comment. Updated release notes in d11a9018b773c0359934a7989d886b02468112e4 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67983/new/ https://reviews.llvm.org/D6

[PATCH] D69876: Allow output constraints on "asm goto"

jyknight added a comment. I think -Wuninitialized (UninitializedValues.cpp) should be taught how to detect the use of output variables in error blocks, at least for trivial cases. Actually, for some reason -- it looks like the warning is failing the wrong way right now, and emits an uninitializ

[PATCH] D69876: Allow output constraints on "asm goto"

jyknight added a comment. Also, since this means we are no longer simply implementing according to GCC's documentation, I think this means we'll need a brand new section in the Clang docs for its inline-asm support. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D67573: Fix __atomic_is_lock_free's return type.

jyknight added a comment. Ping! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67573/new/ https://reviews.llvm.org/D67573 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/c

[PATCH] D70157: Align branches within 32-Byte boundary

jyknight added a comment. In D70157#1747551 , @davezarzycki wrote: > In D70157#1747510 , @xbolva00 wrote: > > > > Even though core2 isn't affected by the erratum, core2 code can run on > > > CPUs that do have the

[PATCH] D70157: Align branches within 32-Byte boundary

jyknight added a comment. Overall comment: this whole change needs more comments, everywhere. Both for the added functions, and for the test cases. There is almost no description of what's happening, and it could really use it. Comment at: llvm/lib/MC/MCAssembler.cpp:1041 + +

[PATCH] D70157: Align branches within 32-Byte boundary

jyknight added a comment. Thanks for the comments, they help a little. But it's still somewhat confusing, so let me write down what seems to be happening: - Before emitting every instruction, a new MCMachineDependentFragment is now emitted, of one of the multiple types: - For most instruction

[PATCH] D70157: Align branches within 32-Byte boundary

jyknight added a comment. I have some other concerns about the code itself, but after pondering this a little bit, I'd like to instead bring up some rather more general concerns about the overall approach used here -- with some suggestions. (As these comments are not really comments on the _cod

[PATCH] D70157: Align branches within 32-Byte boundary

jyknight added a comment. In D70157#1771832 , @reames wrote: > I've been digging through the code for this for the last day or so. This is > a new area for me, so it's possible I'm off base, but I have some concerns > about the current design. > > First

[PATCH] D70157: Align branches within 32-Byte boundary

jyknight added a comment. > .push_align_branch_boundary [N,] [instruction,]* I'd like to raise again the possibility of using a more general region directive to denote "It is allowable to add prefixes/nops before instructions in this region if the assembler wants to", as I'd started discussing

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

jyknight added a comment. In D70157#1788418 , @reames wrote: > In D70157#1788025 , @jyknight wrote: > > > > .push_align_branch_boundary [N,] [instruction,]* > > > > I'd like to raise again the possibility of using a

[PATCH] D75951: Keep a list of already-included pragma-once files for mods.

jyknight added a comment. I suspect we also need to support saving/loading some of this information in the serialized AST, e.g. clang/lib/Serialization/ASTWriter.cpp has code to save the HeaderInfo data, around line 1650. And around line 2174, code to save the macros per submodule. We'll also n

[PATCH] D72742: Don't assume promotable integers are zero/sign-extended already in x86-64 ABI.

jyknight added a comment. I wrote some stuff on https://bugs.llvm.org/show_bug.cgi?id=44228, probably best to continue the discussion there. I really don't think we should take this patch -- at least not without reopening the ABI discussion first. Repository: rG LLVM Github Monorepo CHANGES

<    3   4   5   6   7   8   9   >