[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

jyknight added a comment. Starting by looking at the test cases, I've got some suggestions on making the diagnostics a bit less confusing. Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8 -static int func(f) +static int func(f) // expected-warning {{this function decl

[PATCH] D124435: [X86] Always extend the integer parameters in callee

jyknight added a comment. Thinking more about the naming issue, I think the real issue is that the proposed options are an implementation detail, not what we should expose to users. I realize that the non-boolean option was proposed by rjmmccall before, but I'd suggest that we go back to someth

[PATCH] D124435: [X86] Always extend the integer parameters in callee

jyknight added a comment. In D124435#3696862 , @rjmccall wrote: > That is an interesting idea. I like that it integrates this into > `-fclang-abi-compat`. The way that `-mno-conservative-small-integer-abi` > ends up meaning opposite things based on th

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

jyknight added a comment. I've filed a bug against Boost MPL, https://github.com/boostorg/mpl/issues/69 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130058/new/ https://reviews.llvm.org/D130058 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D124435: [X86] Always extend the integer parameters in callee

jyknight added a comment. In D124435#3697259 , @rjmccall wrote: > I know what you're saying, but I don't think it matches any model of how > programmers use command line flags. You're imagining that a programmer sits > down and considers all of their f

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

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

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

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

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

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

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

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

jyknight added a comment. In D118511#3371432 , @tstellar wrote: > I'm fine with reverting if you think this is the best solution. I just would > like to conclude soon so I can make the final release candidate. ISTM that reverting the ABI change in the

[PATCH] D119271: clang: emit allocalign to LLVM for alloc_align attributes

jyknight added inline comments. Comment at: clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp:55 // CHECK-NEXT:%[[ALIGNMENT_RELOADED:.*]] = load i64, i64* %[[ALIGNMENT_ADDR]], align 8 - // CHECK-NEXT:

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight added a comment. Ping; any more comments? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120159/new/ https://reviews.llvm.org/D120159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D121328: Disable -Wmissing-prototypes for internal linkage functions that aren't explicitly marked "static"""

jyknight added a comment. Given that this is an error `error: functions cannot be declared in an anonymous struct` (thus you can assume that function declarations inside a struct imply that the struct MUST have a linkage name) struct { static int foo(); }; then I don't think there's an

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

jyknight added a comment. Herald added a subscriber: StephenFan. Herald added a project: All. What's the status of this? Did the GCC proposal go anywhere? I'd be happy to see this move forward if you're also pushing it on the GCC side. Comment at: clang/lib/CodeGen/CGStmt.cpp:

[PATCH] D121556: [randstruct] Add randomize structure layout support

jyknight added inline comments. Comment at: clang/unittests/AST/RandstructTest.cpp:154-158 +#ifdef _WIN32 + const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"}; +#else + const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"}; +#endif ---

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

jyknight added inline comments. Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8 -static int func(f) +static int func(f) // expected-warning {{this function declaration without a prototype is deprecated in all versions of C and changes behavior in C2x}} void *f;

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

jyknight added inline comments. Comment at: clang/test/Sema/knr-def-call.c:15 +void f2(float); // expected-note{{previous declaration is here}} \ + expected-warning {{this function declaration with a prototype changes behavior in C2x because it is followed by a

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

jyknight added a comment. Looks reasonable overall. I've added a few specific comments to some tests, but in general, I think we should avoid adding -std=c99 to test-cases to workaround implicit decl issues, unless: - the test is explicitly testing the behavior of implicit decls (potentially i

[PATCH] D123627: Correctly diagnose prototype redeclaration errors in C

jyknight accepted this revision. jyknight added inline comments. Comment at: clang/test/Sema/warn-deprecated-non-prototype.c:64-70 // FIXME: we get two diagnostics here when running in pedantic mode. The first // comes when forming the function type for the definition, and the

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

jyknight added a comment. I disagree with this on principle -- IMO, it is basically a historical bug in GCC that it ignores the type alignment, and we should NOT try to match that -- it's dangerous. We ought to resolve the bug via other fixes: - As a workaround: add alignas(uint64_t) to the af

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

jyknight added a comment. In D123642#3450110 , @efriedma wrote: >> I disagree with this on principle -- IMO, it is basically a historical bug >> in GCC that it ignores the type alignment, and we should NOT try to match >> that -- it's dangerous. > > gcc

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

jyknight added a comment. In D122983#3454406 , @aaron.ballman wrote: > In D122983#3452994 , @rsmith wrote: > >> I think we should just make this an error by default in C99 onwards; > > Out of curiosity -- do you

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

jyknight accepted this revision. jyknight added a comment. Looks good to me, whichever way you go regarding rsmith's comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122983/new/ https://reviews.llvm.org/D122983 ___ cfe-commits mailing

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

[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] 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/ https://reviews.llvm.org/D1136

[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: clang/include/clang/Basic/Tar

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

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

jyknight added a comment. In D118804#3302675 , @rsmith wrote: > I support this revert. Having received enough support, I'll go ahead and commit, and then propose backport to llvm 14 branch. But -- > - `malloc` always returns storage whose alignment is

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9545976ff160: Revert "[Clang] Propagate guaranteed alignment for malloc and others" (authored by jyknight). Repository: rG LLVM Github Monorepo C

[PATCH] D119271: CGCall: also emit LLVM `allocalign` attribute when handling AllocAlign

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:4605 + TryEmitAsCallSiteAttribute(const llvm::AttributeList &Attrs) { +llvm::AttributeList NewAttrs = Attrs; +if (AA) We do need to fallback to an assume for "if(CGF.SanOpts.has(Sa

[PATCH] D119707: [C2x] Implement WG14 N2764 the [[noreturn]] attribute

jyknight added inline comments. Comment at: clang/lib/Headers/stdnoreturn.h:19 +/* The noreturn macro is deprecated in C2x. */ +#pragma clang deprecated(noreturn) + Is this actually useful? Isn't it sufficient to have the include-time warning for this header?

[PATCH] D119707: [C2x] Implement WG14 N2764 the [[noreturn]] attribute

jyknight added inline comments. Comment at: clang/lib/Headers/stdnoreturn.h:22 +/* Including the header file in C2x is also deprecated. */ +#warning "the '' header is deprecated in C2x" +#endif aaron.ballman wrote: > jyknight wrote: > > Would be nice to mention t

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:906 // member, only a T[0] or T[] member gets that treatment. + // Under StrictFlexArraysLevel, obey c99+ that disallows FAM in union, see + // C11 6.7.2.1 §18 serge-sans-p

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added a comment. In D126864#3645994 , @kees wrote: > I should clarify: I still need the =3 mode. Since sizeof([0]) == 0 and > sizeof([]) == error, they are being treated differently already by the > compiler causing bugs in Linux. The kernel mu

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added a comment. Just to move things forward here: I propose to continue and land this patch without mode 3 (there were only a couple comments left to address for that), and continue the discussion about whether to add mode 3 elsewhere. (I don't mind where, KSPP, gcc, or llvm issue tra

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

jyknight added a comment. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697 is the relevant discussion thread that added these to GCC. I personally never found it very convincing, even back in 2015 when they first made the change. And, now, 7 years later, I'd be even more reluctant to add this

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

jyknight added a comment. Sorry if I was unclear -- I did intend that the remaining minor open comments be addressed before commit. I've quoted them again here for clarity. Please address in a follow-up, thanks! Comment at: clang/include/clang/Driver/Options.td:1143 Marsha

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

jyknight added a comment. In D129802#3654943 , @Wilco1 wrote: > The general requirement is that inline and outline atomics have identical > behaviour, and that GCC and LLVM emit the same sequences. I agree sync is > badly documented, so it's hard to fig

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

jyknight added a comment. The warnings for this case aren't great: int foo(); int foo(int arg) { return 5; } results in: /tmp/test.c:1:5: warning: a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-proto

[PATCH] D124435: [X86] Always extend the integer parameters in callee

jyknight added a comment. I find the option names you have a bit confusing. I'd like to suggest calling them, instead: caller: Extend a small integer parameter in the caller; callee will assume it has already been extended. callee : Pass a small integer parameter directly in caller, extend in c

[PATCH] D125814: Fix strict prototype diagnostic wording for definitions

jyknight added a comment. This confuses me. Looking at behavior with default flags: We won't emit a -Wdeprecated-non-prototype warning for `int foo();`, until we subsequently find `int foo(int arg) { return 5; }`. Since we definitely have the context of what's going on at that point, in order

[PATCH] D125773: [Driver] Do not auto-enable header modules with -std=c++20

jyknight added a comment. In D125773#3523459 , @rsmith wrote: > Header modules are part of the C++20 standard (where they are called "header > units"), and module maps are an intended way for Clang to provide this > functionality in C++20 mode. I don't

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

jyknight created this revision. jyknight added reviewers: aaron.ballman, rsmith. Herald added a project: All. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Const class members may be initialized with a defaulted default construct

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

jyknight added a comment. That GCC and Clang differ in handling of Atomics is a really unfortunate, longstanding issue. See https://bugs.llvm.org/show_bug.cgi?id=26462 For RISCV, perhaps it's not yet too late to have the RISCV psABI actually specify a single ABI for C11 _Atomic which all compi

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

jyknight added a comment. In D57450#1641190 , @lenary wrote: > @jyknight I hear where you're coming from. I'll see what I can do about the > psABI document. > > In that ticket, it's mentioned that the Darwin ABI explicitly says that > non-power-of-two at

[PATCH] D66822: Hardware cache line size builtins

jyknight added a comment. numbers for cacheline size. In D66822#1647664 , @zoecarver wrote: > > Passing-by remark: i'm not sure it is possible to **guarantee** that this > > will be always correct and that no ABI break will happen. > > You're right. I sh

[PATCH] D34158: For standards compatibility, preinclude if the file is available

jyknight added a comment. In https://reviews.llvm.org/D34158#823316, @fedor.sergeev wrote: > Hmm... I tried this patch and now the following worries me: > > - it passes -finclude-if-exists stdc-predef.h on all platforms (say, > including my Solaris platform that has no system stdc-predef.h) Ri

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

jyknight added a comment. In https://reviews.llvm.org/D34158#827178, @joerg wrote: > I had a long discussion with James about this on IRC without reaching a clear > consensus. I consider forcing this behavior on all targets to be a major bug. > It should be opt-in and opt-in only: > > (1) The h

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

jyknight added a comment. Just to restate: the ideal outcome of this discussion for me would be to resolve things such that _ALL_ libc implementations will feel comfortable using this technique to provide the C11-required predefined macros. I'd love for linux, freebsd, macos, solaris, etc etc l

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

jyknight added a comment. In https://reviews.llvm.org/D34158#838454, @mibintc wrote: > This patch is responding to @jyknight 's concern about preprocessed input. > The patch as it stands doesn't have this issue. I added 2 test cases, one > using option -x cpp-output, and another for a source fi

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

jyknight accepted this revision. jyknight added a comment. LGTM modulo some minor wording. Comment at: clang/docs/LanguageExtensions.rst:1256-1258 +Clang provides support for the `goto form of GCC's extended +assembly`_ with

[PATCH] D71848: Allow the discovery of Android NDK's triple-prefixed binaries.

jyknight added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4696 + llvm::Triple ToolchainTriple = TC.getTriple(); + if (ToolchainTriple.isAndroid()) { +std::string ArchName = ToolchainTriple.getArchName(); Adding the hardcoding here seems unfort

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

jyknight added a comment. I think you pushed the incorrect change? This was the clang half, but now it's showing the LLVM half. Can you re-upload the diff for the clang half? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69876/new/ https://reviews

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

jyknight added inline comments. Comment at: clang/lib/AST/Stmt.cpp:646-648 + // Labels are placed after "InOut" operands. Adjust accordingly. + if (IsLabel) +N += getNumPlusOperands(); void wrote: > jyknight wrote: > > I'm confused about this pa

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

jyknight reopened this revision. jyknight added a comment. This revision is now accepted and ready to land. Reopening, since this didn't actually land yet. BTW, this review still has the wrong contents in the latest uploaded-diff (showing llvm changes, not clang changes). Repository: rG LLVM

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

jyknight added inline comments. Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701 + } else if (MBB->succ_size() == LandingPadSuccs.size() || + MBB->succ_size() == IndirectPadSuccs.size()) { // It's possible that the block legitimately ends with a

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

jyknight added inline comments. Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701 + } else if (MBB->succ_size() == LandingPadSuccs.size() || + MBB->succ_size() == IndirectPadSuccs.size()) { // It's possible that the block legitimately ends with a

[PATCH] D71848: Allow the discovery of Android NDK's triple-prefixed binaries.

jyknight added a comment. Since this change is not android-only, please update change description, and update some non-android tests. E.g. maybe the tests in clang/test/Driver/linux-ld.c for ubuntu_14.04_multiarch_tree should check the path at which ld is found. While the existing Inputs/ubunt

[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

jyknight added inline comments. Herald added a project: clang. Comment at: lib/Headers/__stddef_max_align_t.h:44 +#endif } max_align_t; #endif EricWF wrote: > rsmith wrote: > > I don't want to hold up the immediate fix in this patch for this, but... we > > sho

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

jyknight added a comment. In D67847#1693694 , @rnk wrote: > In D67847#1691898 , @jyknight wrote: > > > The `abort()` function raises SIGABRT, for which the default behavior is to > > trigger a coredump. Do we actua

[PATCH] D86881: Make -fvisibility-inlines-hidden apply to static local variables in inline functions on Darwin

jyknight added a comment. But this (re-)breaks the functionality of -fvisibility-inlines-hidden on Darwin. That seems bad? I'd've liked to see more of an explanation as to why this was considered a necessary breakage. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D86790: [FE] Use preferred alignment instead of ABI alignment for complete object when applicable

jyknight added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:256 - /// A cache from types to size and alignment information. using TypeInfoMap = llvm::DenseMap; + /// A cache from types to size and ABI-specified alignment information. P

[PATCH] D86790: [FE] Use preferred alignment instead of ABI alignment for complete object when applicable

jyknight added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4539 CharUnits CCAlign = getParamTypeAlignment(Ty); CharUnits TyAlign = getContext().getTypeAlignInChars(Ty); Xiangling_L wrote: > jasonliu wrote: > > Question: > > It looks

[PATCH] D86790: [FE] Use preferred alignment instead of ABI alignment for complete object when applicable

jyknight added a comment. I'm happy with this now, but please update the commit message to match the updated change. Comment at: clang/include/clang/AST/ASTContext.h:2177-2179 /// This can be different than the ABI alignment in cases where it is - /// beneficial for perfor

[PATCH] D86790: [FE] Use preferred alignment instead of ABI alignment for complete object when applicable

jyknight added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4539 CharUnits CCAlign = getParamTypeAlignment(Ty); CharUnits TyAlign = getContext().getTypeAlignInChars(Ty); efriedma wrote: > jyknight wrote: > > Xiangling_L wrote: > > > j

[PATCH] D88195: Remove stale assert.

jyknight added inline comments. Comment at: clang/test/Modules/Inputs/asm-goto/a.h:4-6 + asm goto("xor $0, $0\n\t" + "test $0, $0\n\t" + "jne $l1\n\t" An empty asm string will suffice for the test. Comment at: clang/test/Mo

[PATCH] D88195: Remove stale assert.

jyknight added inline comments. Comment at: clang/test/Modules/asm-goto.cpp:1 +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x c++ -I%S/Inputs/asm-goto -emit-module %S/Inputs/asm-goto/module.modulemap -fmodule-name=a -o %t/a.pcm void wro

[PATCH] D88195: Remove stale assert.

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. In D88195#2293597 , @nickdesaulniers wrote: > In D88195#2293589 , @void wrote: > >> Clarify commit message

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

jyknight added a comment. BTW, to the apple folks here, it'd sure be awesome if this could be backported into XCode 12's clang. :) (c.f. FB7037642). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82777/new/ https://reviews.llvm.org/D82777

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

jyknight created this revision. jyknight added reviewers: craig.topper, spatel, RKSimon. Herald added subscribers: cfe-commits, danielkiss. Herald added a project: clang. jyknight requested review of this revision. Preliminary patch, posted to go along with discussion on llvm-dev. 3DNow! intrinsi

[PATCH] D86790: [FE] Use preferred alignment instead of ABI alignment for complete object when applicable

jyknight added a comment. Do you have open questions on whether some callsites passing "false" here, should be switched to true? Given what's here, I would say that it definitely does not makes sense to add this parameter everywhere. So, for getting something committed: please send a new review

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

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

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

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

[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"); + CmdArgs.push_back

[PATCH] D88659: [FE]Split SuitableAlign into two parts

jyknight added a comment. Hm, to start with, the current state of this confuses me. In GCC, the preprocessor macro `__BIGGEST_ALIGNMENT__` was supposed to expose the alignment used by `__attribute__((aligned))` (no arg specified), as well the alignment used for alloca. However, this is no longe

[PATCH] D88659: [FE]Split SuitableAlign into two parts

jyknight added a comment. > As you mentioned, without this patch, `SuitableAlign` is used for the > predefined `__BIGGEST_ALIGNMENT__` and alignment for alloca. But on AIX, the > __BIGGEST_ALIGNMENT__ should be 8bytes, alloca and > `__attribute__((aligned))` should align to 16bytes considerin

[PATCH] D88659: [FE]Split SuitableAlign into two parts

jyknight added a comment. In D88659#2319636 , @hubert.reinterpretcast wrote: > In D88659#2318203 , @jyknight wrote: > >> It seems like on AIX, `__BIGGEST_ALIGNMENT__` should just be set to 16, >> then. I'm not sur

[PATCH] D88659: [FE]Split SuitableAlign into two parts

jyknight added a comment. In D88659#2339357 , @Xiangling_L wrote: > Hi @jyknight , are you okay with us changing the "definition" of > SuitableAlign without sending a note to the mailing list? Yes, I think that it is fine to adjust the comment just in th

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

jyknight added a comment. In D71726#2207700 , @yaxunl wrote: > clang does not always emit atomic instructions for atomic builtins. Clang may > emit lib calls for atomic builtins. Basically clang checks target info about > max atomic inline width and if t

[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

<    2   3   4   5   6   7   8   9   >