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

jyknight added a comment. Oh, one more note, C11 has -- and clang already supports -- `_Atomic long double x; x += 4;` via lowering to a cmpxchg loop. Now that we have an LLVM IR representation for atomicrmw fadd/fsub, clang should be lowering the _Atomic += to that, too. (Doesn't need to be

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

jyknight added a comment. In D71726#2182667 , @tra wrote: >> If a target would like to treat single and double fp atomics as unsupported, >> it can override the default behavior in its own TargetInfo. I really don't think this should be a target option

[PATCH] D82782: Clang Driver: refactor support for writing response files to be specified at Command creation, rather than as part of the Tool.

This revision was automatically updated to reflect the committed changes. Closed by commit rG4772b99dffec: Clang Driver: refactor support for writing response files to be specified at… (authored by jyknight). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D82777: Clang Driver: Use Apple ld64's new @response-file support.

This revision was automatically updated to reflect the committed changes. Closed by commit rG381df1653c92: Clang Driver: Use Apple ld64s new @response-file support. (authored by jyknight). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82777/new/

[PATCH] D82777: Clang Driver: Use Apple ld64's new @response-file support.

jyknight created this revision. jyknight added reviewers: steven_wu, arphaman. Herald added subscribers: cfe-commits, dexonsmith. Herald added a project: clang. jyknight added a child revision: D82782: Clang Driver: refactor support for writing response files to be specified at Command creation,

[PATCH] D82782: Clang Driver: refactor support for writing response files to be specified at Command creation, rather than as part of the Tool.

jyknight created this revision. jyknight added reviewers: rnk, arphaman, steven_wu. Herald added subscribers: cfe-commits, sstefan1, kerbowa, luismarques, apazos, sameer.abuasal, pzheng, s.egerton, lenary, Jim, mstorsjo, jocewei, PkmX, dexonsmith, the_o, brucehoult, MartinMosbeck, rogfer01,

[PATCH] D83015: [Driver] Add -fld-path= and deprecate -fuse-ld=/abs/path and -fuse-ld=rel/path

jyknight added a comment. BTW, I just noticed recently that we have a -mlinker-version= flag, too, which is only used on darwin at the moment. That's another instance of "we need to condition behavior based on what linker we're invoking", but in this case, between multiple versions of apple's

[PATCH] D81313: Fix handling of constinit thread_locals with a forward-declared type.

jyknight created this revision. jyknight added a reviewer: rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. ItaniumCXXABI::usesThreadWrapperFunction calls VarDecl::needsDestruction, which calls QualType::isDestructedType, which checks

[PATCH] D81432: Add #includes so that ROCm.h is compilable stand-alone.

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

[PATCH] D81772: Reduce -Wregister from DefaultError to a default-on warning.

jyknight created this revision. jyknight added a reviewer: rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. There is a lot of C++ code in the wild still using 'register', which will become broken if we switch the default compilation mode to C++17. A default-on

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

jyknight created this revision. jyknight added reviewers: craig.topper, spatel, RKSimon. Herald added subscribers: dang, pengfei. jyknight requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. This set of instructions was only

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

jyknight marked 7 inline comments as done. jyknight added a comment. Herald added a subscriber: pengfei. I've finally got back to moving this patch forward -- PTAL, thanks! To start with, I wrote a simple test-suite to verify the functionality of these changes. I've included the tests I wrote

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

jyknight created this revision. jyknight added reviewers: craig.topper, spatel, RKSimon. jyknight requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. After switching the headers to implement the intrinsics using SSE2 (see

[PATCH] D94268: Allow _mm_empty() (via llvm.x86.mmx.emms) to be a no-op without MMX.

jyknight created this revision. jyknight added reviewers: craig.topper, spatel, RKSimon. Herald added subscribers: pengfei, hiraditya. jyknight requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. In Clang, the other "MMX"

[PATCH] D91311: Add new 'preferred_name' attribute.

jyknight added inline comments. Comment at: clang/include/clang/Basic/Attr.td:2384 + let Spellings = [Clang<"preferred_name", /*AllowInC*/0>]; + let Subjects = SubjectList<[ClassTmpl]>; + let Args = [TypeArgument<"TypedefType">]; I wonder if this attribute

[PATCH] D92662: [Clang][Coroutine] Drop const attribute on pthread_self when coroutine is enabled

jyknight added a comment. I don't think we should change the meaning of `__attribute__((const))` to exclude depending on thread-id. However, if we do want to do so, and call the existing uses of `__attribute__((const))` in glibc invalid, we need to special case many more functions. Looking

[PATCH] D93072: Fix PR35902: incorrect alignment used for ubsan check.

jyknight created this revision. jyknight added reviewers: rsmith, rnk. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. UBSan was using the complete-object align rather than nv alignment when checking the "this" pointer of a

[PATCH] D91157: [AArch64] Out-of-line atomics (-moutline-atomics) implementation.

jyknight added inline comments. Comment at: llvm/docs/Atomics.rst:625 + +Libcalls: out-of-line atomics += I think this section needs to be put on the end of the section on `__sync_*`. These functions are effectively an

[PATCH] D91157: [AArch64] Out-of-line atomics (-moutline-atomics) implementation.

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. LG after fixing the minor nits. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6377 +} else { + CmdArgs.push_back("-target-feature"); +

[PATCH] D93072: Fix PR35902: incorrect alignment used for ubsan check.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4ddf140c0040: Fix PR35902: incorrect alignment used for ubsan check. (authored by jyknight). Changed prior to commit:

[PATCH] D94268: Allow _mm_empty() (via llvm.x86.mmx.emms) to be a no-op without MMX.

jyknight added a comment. In D94268#2485958 , @pengfei wrote: > Is inline assembly the only case `emms` instruction will be needed? But > inline assembly doesn't enable `mmx` attribute automatically, right? E.g. > https://godbolt.org/z/43ases Yes,

[PATCH] D94268: Allow _mm_empty() (via llvm.x86.mmx.emms) to be a no-op without MMX.

jyknight abandoned this revision. jyknight added a comment. OK thanks -- abandoning this patch. I'll adjust the comment on _mm_empty to mention that it's no longer necessary except with asm in the intrinsics patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

jyknight added a comment. Agreed w.r.t. timing -- I would like to get all of these changes landed soon after the LLVM 12 branch-cut, to allow plenty time to stew on the main branch before they make it into a release to try to identify any issues. I'd still appreciate review and approval with

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

jyknight added a comment. Was the "group" libc++abi accept intended to be an accept for the whole revision? Or should still ping for review from rsmith or someone else? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93922/new/

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

jyknight added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:4009 +// mangling. Previously, it used a special-cased nonstandard extension. +if (Context.getASTContext().getLangOpts().getClangABICompat() >= +LangOptions::ClangABI::Ver11) {

[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:

[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: Mangle `__alignof__` differently than `alignof`.

jyknight created this revision. jyknight added reviewers: rsmith, rjmccall. jyknight requested review of this revision. Herald added projects: clang, libc++abi, LLVM. Herald added subscribers: llvm-commits, libcxx-commits, cfe-commits. Herald added a reviewer: libc++abi. The two operations have

[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

[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] 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,

[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

[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 expr-primary in template-arg. (authored by jyknight). Changed prior to commit:

[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

[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 expr-primary. (authored by jyknight). Repository: rG LLVM Github Monorepo CHANGES

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

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

[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

[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 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

[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

[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] 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

[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

[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

[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:

[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); }

[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:

[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

[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] D17183: Make TargetInfo store an actual DataLayout instead of a string.

jyknight added a comment. In general, I think it's extremely unfortunate that Clang and LLVM have different copies of the same information. It's a problem for way more than just this one situation. So, I really don't like choice 1 -- I think it's moving in the wrong direction. The recent

[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

[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:

[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

[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

[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:

[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

[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

[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

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

jyknight added a comment. In D99790#2677917 , @brooksmoses wrote: > As a heads up, I'm seeing segfaults on internal code as a result of this > change, as well as errors in Eigen's unalignedassert.cpp test (specifically, > this line asserts: >

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

jyknight added a comment. The OpenJDK bug was UB in the OpenJDK code: https://bugs.openjdk.java.net/browse/JDK-8229258 already fixed in JDK14. (But not backported to JDK 11 LTS, which is the version Brooks found an error in above.) They probably need to backport that patch. My only confusion

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

jyknight added a comment. It seems like there's a bug with vtable thunks getting the wrong information. This appears to be a pre-existing bug, but this change has caused it to start actively breaking code. Test case: class Base1 { virtual void Foo1(); }; class Base2 {

[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

[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

[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,

[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

[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

[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

[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

[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] 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

[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

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

jyknight added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:581 /// limitation is put into place for ABI reasons. - virtual bool hasExtIntType() const { + /// FIXME: _BitInt is a required type in C23, so there's not much utility in + /// asking

[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

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. In D106577#2944086 , @aaron.ballman wrote: >> I don't think that scenario is valid. MBCS-to-unicode mappings are a part of >> the definition of the MBCS (sometimes officially, sometimes de-facto defined >> by major vendors),

[PATCH] D108243: Put code that avoids heapifying local blocks behind a flag

jyknight added a comment. In D108243#2995898 , @waltl wrote: > Added driver flags, and tests for them @ahatanak did you intend to ask Walt to add a driver flag for this? I think we should not have one, since this isn't something we should be telling

[PATCH] D108243: Put code that avoids heapifying local blocks behind a flag

jyknight accepted this revision. jyknight added inline comments. Comment at: clang/include/clang/Driver/Options.td:2380 + NegFlag, + BothFlags<[CC1Option], " to avoid heapifying local blocks">>; Needs to be marked `[CC1Option, NoDriverOption]` Repository:

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

jyknight added a comment. Ping. Any comment from Apple folks? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108243/new/ https://reviews.llvm.org/D108243 ___ cfe-commits mailing list

[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

[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=386326#toc Repository: rG LLVM

[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] 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] 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

[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

[PATCH] D87279: [clang] Fix handling of physical registers in inline assembly operands.

jyknight added a comment. This change seems like it's done in the wrong layer -- why do we add a special-case in Clang to emit "={r11},{r11}", instead of fixing LLVM to not break when given the previously-output "={r11},0"? Furthermore, since earlyclobber was exempted from this change, doesn't

[PATCH] D115471: [clang] number labels in asm goto strings after tied inputs

jyknight added a comment. It's rather sad that GCC made the quite-unintuitive decision to number the arguments in this way -- LONG AFTER clang had already implemented the other way... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115471/new/

[PATCH] D115410: [llvm][test] rewrite callbr to use i rather than X constraint NFC

jyknight added inline comments. Comment at: llvm/test/tools/llvm-diff/callbr.ll:28-29 entry: - callbr void asm sideeffect "", "X,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %t_no), i8* blockaddress(@foo, %return)) + callbr void asm sideeffect "",

[PATCH] D115253: [C2x] Support the *_WIDTH macros in limits.h and stdint.h

jyknight added inline comments. Comment at: clang/lib/Frontend/InitPreprocessor.cpp:900-903 + Builder.defineMacro("__USHRT_WIDTH__", Twine(TI.getShortWidth())); + Builder.defineMacro("__UINT_WIDTH__", Twine(TI.getIntWidth())); + Builder.defineMacro("__ULONG_WIDTH__",

[PATCH] D115409: [SelectionDAGBuilder] drop special handling for CallBr

jyknight added a comment. OK, I do think that special case definitely needs to be deleted. It's assuming that the block args are in a particular place in the argument list of the callbr -- the place Clang put them -- and then assigned special behavior based on that location. But I think it

[PATCH] D115410: [llvm][test] rewrite callbr to use i rather than X constraint NFC

jyknight added a comment. This looks good to me, but I'd like to wait for a conclusion on D115409 first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115410/new/ https://reviews.llvm.org/D115410

[PATCH] D115311: [clang][CGStmt] emit i constraint rather than X for asm goto indirect dests

jyknight added a comment. This looks reasonable to me, but I'd like to wait for a conclusion on D115409 first. (Which probably will result in rewriting the commit message, as well). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D115410: [llvm][test] rewrite callbr to use i rather than X constraint NFC

jyknight added inline comments. Comment at: llvm/test/Verifier/callbr.ll:42 + callbr void asm sideeffect "${0:l} ${1:l} ${2:l}", "i,X,i"(i8* blockaddress(@test3, %4), i8* blockaddress(@test3, %2), i8* blockaddress(@test3, %3)) to label %1 [label %3, label %4] 1:

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. I'm not sure we should be populating this. The _value_ is determined by what libc supports, so it probably needs to be left up to libc to define it. In glibc, this define is set by the file /usr/include/stdc-predef.h, which GCC implicitly includes into all TUs

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. In D106577#2897588 , @aaron.ballman wrote: > In D106577#2897522 , @jyknight > wrote: > >> I'm not sure we should be populating this. >> >> The _value_ is determined by what libc

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. Even after the more recent discussion, I still think my initial message was incorrect, and that the compiler should be defining this macro itself, as proposed in this patch. Note that my confusion was not that the macro being defined or not was dependent on libc

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. In D106577#2901654 , @rsmith wrote: > I think this patch as it stands would cause problems with libc > implementations that put their `#define` somewhere other than than > `stdc-predef.h` (eg, older versions of glibc that put

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. BTW, looks like the standard wording came from: http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_273.htm which indeed seems to suggest that the intent was to: 1. ensure that WCHAR_MAX is at least the maximum character actually defined so far by the standard (which

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. Perhaps a reasonable path forward here to address the BSD issue can be to add a targetinfo method: /* Returns true if the expected encoding of wchar_t changes at runtime depending on locale for this target. Note that clang always encodes wide character

[PATCH] D106577: [clang] Define __STDC_ISO_10646__

jyknight added a comment. In D106577#2904960 , @rsmith wrote: > One benefit we don't get with this approach is providing the right value for > the macro (without paying the cost of always including `stdc-predefs.h`). What do you mean by "right value",

<    1   2   3   4   5   >