[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3301746 , @Lapenkov wrote: > Is it hard to retrofit this diff into LLVM 13? It would be easy to backport this change into LLVM 13, but the problem is that there are no more releases planned on the LLVM 13 branch.

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight updated this revision to Diff 406478. jyknight added a comment. Rebase, and preserve (and modify) test-case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/ https://reviews.llvm.org/D118804 Files:

[PATCH] D113620: Skip exception cleanups when the innermost scope is EHTerminateScope.

jyknight added a comment. Should be fixed by rGcaa1ebde70673ddb7124a0599ba846362a1f8b1e . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113620/new/

[PATCH] D113620: Skip exception cleanups when the innermost scope is EHTerminateScope.

jyknight added a comment. Found the problem, tweaking a test-case; will commit shortly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113620/new/ https://reviews.llvm.org/D113620 ___ cfe-commits mailing

[PATCH] D110869: [X86] Implement -fzero-call-used-regs option

jyknight added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:6271 +This attribute, when attached to a function, causes the compiler to zero a +subset of all call-used registers before the function returns. It's used to +increase program security by either

[PATCH] D110869: [X86] Implement -fzero-call-used-regs option

jyknight added a comment. In D110869#3295477 , @nathanchance wrote: > Rather interesting function to have problems with as a result of this patch > but it seems like this function is being used in a very specific way further > down the file with the

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3292185 , @ychen wrote: > I don't see why the patch is wrong... It uses the target/platform-specific > `NewAlign`. If the platform allows customized memory allocation that assumes > weak alignment, it should set the

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. > For C++, I confess I have some problems interpreting this sentence: > > (https://eel.is/c++draft/cpp.predefined) > >> `__STDCPP_­DEFAULT_­NEW_­ALIGNMENT__`: An integer literal of type >> std::size_t whose value is the alignment guaranteed by a call to operator >>

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a subscriber: collares. jyknight added a comment. In D118804#3290874 , @collares wrote: > might be worth filing a GCC bug as well Yes, a bug report for GCC should be opened as well. @collares do you want to take that? In D118804#3290937

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3290874 , @collares wrote: > It is worth noting that GCC started assuming 16-byte alignment for small > objects in 2016, before N2293 was written and accepted into C2x; see >

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight added a comment. In D118804#3290803 , @xbolva00 wrote: > GCC also does same assumptions Looking at GCC: GCC only assumes 4-byte alignment on i386, and 8-byte alignment on x86-64, which is why it hasn't actively broken users, unlike the clang

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight updated this revision to Diff 405315. jyknight added a comment. (fix git mishap: neglected to add a file to original change after conflict resolution) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118804/new/

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

jyknight created this revision. jyknight added reviewers: aaron.ballman, rjmccall, jdoerfert, xbolva00. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The above change assumed that malloc (and friends) would always allocate

[PATCH] D116544: [Clang] Introduce Clang Linker Wrapper Tool

jyknight added a comment. "clang-linker-wrapper" seems like a very generic name for a command which is OpenMP offloading specific? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116544/new/ https://reviews.llvm.org/D116544

[PATCH] D118297: [clang] add Diag -Wasm-volatile for implied volatile asm stmts

jyknight added a comment. I agree that's the expected semantics. I think those semantics are unfortunate, but they're not gonna change. IMO it would've been better if you had to opt-in to "no side effects" via `__attribute__((const))` or so. But I wonder why you think we should be discouraging

[PATCH] D117238: [C2x] Add BITINT_MAXWIDTH support

jyknight accepted this revision. jyknight added a comment. No additional comments, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117238/new/ https://reviews.llvm.org/D117238 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D117304: [clang][dataflow] Remove TestingSupport's dependency on gtest

jyknight added a comment. Herald added a subscriber: steakhal. > Users outside of the clang repo may use different googletest versions. I don't understand what that means. Why does it matter what version outside users are using -- these are clang unit-tests, not a public API, right?

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

jyknight accepted this revision. jyknight added a comment. Accepting assuming the last comment will be addressed before pushing. Thanks! Comment at: clang/lib/Frontend/InitPreprocessor.cpp:257 + if (IsSigned) +DefineTypeSizeAndWidth("__INT" + Twine(TypeWidth), Ty, TI,

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

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

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] 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: 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: 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] 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] 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] 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] 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. 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#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] 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. 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",

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

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] 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] 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] 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; + }; + rjmccall wrote: > I think it might be more reasonable to just check for the relatively small > number of primary-expression cases in

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

<    1   2   3   4   5   >