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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

jyknight added a comment. This patch as it stands is harmless, since as it only defines an internal interface, which is unused. So in that sense, it's perfectly fine to commit even with the remaining unresolved questions about the correct values to return. However, unless we're going to actuall

[PATCH] D60748: Adds an option "malign-pass-aggregate" to make the alignment of the struct and union parameters compatible with the default gcc

jyknight added a comment. Since the ABI this is trying to match is not documented literally anywhere, I think we need to have some confidence that what this implements is actually the same as what GCC does. While I wrote up what I think the algorithm is, without some sort of script to allow tes

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

jyknight added a comment. In D67983#1863019 , @arphaman wrote: > @jyknight @rjmccall I'm not sure this change is 100% fine. For example, the > following code no longer compiles with ARC: > > @protocol Delegate > @end > > @interface X > > @end

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

jyknight added a comment. >> Your error looks correct to me -- "self" in a classmethod is not an >> instance, but the class itself. And while instances of X implement >> "Delegate", the Class does not. > > Got it, thanks! We might need to add a flag to allow the old behavior > temporarily to a

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

jyknight added a comment. The idea of moving the copies to a new MachineBasicBlock seems a reasonable solution. That said, it does mean there will be allocatable physical registers which are live-in to the following block, which is generally not allowed. As far as I can tell, I _think_ that sho

[PATCH] D144889: [C2x] Support in freestanding

jyknight added a comment. In D144889#4156974 , @rsmith wrote: > Likely because of GCC's perspective on this, the set of C headers provided by > GCC, Clang, ICC, etc. has included the complete list of freestanding headers > and more or less no others, wi

[PATCH] D144889: [C2x] Support in freestanding

jyknight added a comment. > On the other hand, I think a not-insignificant number of users are interested > in freestanding environments for one-off/fun/experimental projects where ease > of access is more important than performance characteristics -- think: users > who are playing around with

[PATCH] D122258: [MC] Omit DWARF unwind info if compact unwind is present where eligible

jyknight added a comment. > no-compact-unwind is particularly useful for newer x86_64 platforms: we don't > want to omit DWARF unwind for x86_64 in general due to possible backwards > compat issues, but we should make it possible for people to opt into this > behavior if they are only targeting

[PATCH] D145416: [clang] model 'p' inline asm constraint as reading memory

jyknight added a comment. It looks to me from GCC that constraint 'p' is intended to be used internally, not for inline-asm. I question whether the kernel should be using it, and whether we should even implement it at all. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D145416: [clang] model 'p' inline asm constraint as reading memory

jyknight added a comment. > ‘p’ in the constraint must be accompanied by address_operand as the predicate > in the match_operand. This predicate interprets the mode specified in the > match_operand as the mode of the memory reference for which the address would > be valid. How do you do that w

[PATCH] D145416: [clang] model 'p' inline asm constraint as reading memory

jyknight added a comment. Also, I note the doc says it's useful for `for “load address” and “push address” instructions` (note, "load address" means e.g. x86 "lea" instruction) -- which should NOT be dependent upon the value stored in the memory. The x86 backend actually uses a "Ts" constraint

[PATCH] D144999: [RFC][MC][MachO]Only emits compact-unwind format for "canonical" personality symbols. For the rest, use DWARFs.

jyknight added a comment. I wonder if we actually need to define a clang frontend flag for this; I suspect nobody will ever want to specify it, since the only non-canonical personality clang will ever generate that changes behavior is the pure-C destructor-only personality, `__gnu_personality_v

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

jyknight added a comment. When looking for errors in existing codebases, don't forget that this diagnostic is currently suppressed by default in system headers. So this patch is moving from "no diagnostics for code in system headers" to "unconditional hard error in system headers". Just removin

[PATCH] D145416: [clang] model 'p' inline asm constraint as reading memory

jyknight requested changes to this revision. jyknight added a comment. This revision now requires changes to proceed. I believe this is abandoned, correct? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145416/new/ https://reviews.llvm.org/D145416

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

jyknight added a comment. In D150226#4400782 , @rupprecht wrote: > As a general question/feature request: is there a way to have specific > warnings apply even for system headers? It would be nice if I could check > what breaks when by adding `-Wsystem-

[PATCH] D148700: [clang] Add support for “regular” keyword attributes

jyknight added a comment. > This makes ``CXX11`` and ``C2x`` spellings > unsuitable for attributes that affect the type system, that change the > binary interface of the code, or that have other similar semantic meaning. Yes, standard attributes aren't supposed to be used for things which affect

[PATCH] D148700: [clang] Add support for “regular” keyword attributes

jyknight added a comment. In D148700#4418767 , @rsandifo-arm wrote: > Hi @jyknight , @rsmith > > Do you have any more thoughts on the above? Quick version is: > > 1. Is it OK to have `[[…]]` attributes in the `arm` namespace that affect > semantics? I

[PATCH] D152401: CGExprCXX: emit family and allockind attributes

jyknight added a comment. > As a result, clang/llvm should correctly > optimize custom allocators like this without needing specialized > knowledge going forward. Not exactly: we still need knowledge of this in //clang// (`isReplaceableGlobalAllocationFunction` was taught that the variant with a

[PATCH] D123085: CGExprCXX: emit allocptr attributes for operator delete

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGExprCXX.cpp:1333 +CalleeDecl->getOverloadedOperator() == OO_Delete) { + CallOrInvoke->addParamAttr(0, llvm::Attribute::AllocatedPointer); +} Perhaps we should add this attribute to t

[PATCH] D121629: clang: also check alloc_alignment claims in return

jyknight added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:3684 + const AllocAlignAttr *AA = CurCodeDecl->getAttr(); + if (SanOpts.has(SanitizerKind::Alignment) && AA) { It may be nice to also verify the alignment required by an AssumeAlignAttr

[PATCH] D139652: Add the thread sanitizer support for X86_64 WatchOS simulators

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. Thanks for the change. The description is a bit misleading, this would be better: Allow TSan in clang driver for X86_64 WatchOS simulator. It was already functional, and Apple's dow

[PATCH] D24688: Introduce "hosted" module flag.

jyknight added a comment. Since this requires everyone generating llvm IR to add this module attribute for optimal codegen, it should be documented in release notes and the LangRef, I think. I mean: it's unfortunate that it's needed at all, but at the very least it should be possible for peopl

[PATCH] D27387: [libc++] Add a key function for bad_function_call

jyknight added a comment. Since this is only adding a new export to libc++, not changing/breaking existing ones, I don't think it should be grouped with the ABI-breaking changes in v2. The usual promise is that upgrading libc++.so.1 will not break apps compiled against an older set of headers,

[PATCH] D30427: Fix whitespace before token-paste of an argument.

jyknight added a comment. Ping. https://reviews.llvm.org/D30427 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31022: Implement P0298R3: `std::byte`

jyknight added a comment. I believe this needs compiler support, too, in order to treat namespace std { enum class byte : unsigned char {}; } as directly having tbaa type "omnipotent char" instead of a subtype. That is, given: void foo(char* x, int *y) { x[1] = char(y[0] & 0xff); x

[PATCH] D31022: Implement P0298R3: `std::byte`

jyknight added a comment. Ping. I don't think this got resolved, and I really wouldn't like to see released in this state...can you either revert this from the library, or implement the compiler support, before the 5.0 release branch? In https://reviews.llvm.org/D31022#716702, @jyknight wrote:

[PATCH] D34158: to support gcc 4.8 (and newer) compatibility on Linux, preinclude

jyknight added inline comments. Comment at: lib/Driver/ToolChains/Linux.cpp:755 +!Version.isOlderThan(4, 8, 0)) { + // For gcc >= 4.8.x, clang will preinclude + // -ffreestanding suppresses this behavior. I don't see why it makes any sense to c

[PATCH] D34158: to support gcc 4.8 (and newer) compatibility on Linux, preinclude

jyknight added inline comments. Comment at: lib/Driver/ToolChains/Linux.cpp:755 +!Version.isOlderThan(4, 8, 0)) { + // For gcc >= 4.8.x, clang will preinclude + // -ffreestanding suppresses this behavior. jyknight wrote: > I don't see why it ma

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

jyknight added a comment. This version is still disabling upon -nostdinc, which doesn't make sense to me. You didn't remove that, nor respond explaining why you think it does make sense? https://reviews.llvm.org/D34158 ___ cfe-commits mailing list

[PATCH] D34294: Rework libcxx strerror_r handling.

This revision was automatically updated to reflect the committed changes. Closed by commit rL308528: Rework libcxx strerror_r handling. (authored by jyknight). Repository: rL LLVM https://reviews.llvm.org/D34294 Files: libcxx/trunk/src/system_error.cpp Index: libcxx/trunk/src/system_error

[PATCH] D42742: [clangd] Use pthread instead of thread_local to support more runtimes.

jyknight added a comment. Is there some way to figure out what's going on in clang-x86_64-linux-selfhost-modules? I believe there should be no linux builders which are missing this function -- it was added to libgcc in 4.8, and we don't support older versions, so I think missing this function

[PATCH] D30427: Fix whitespace before token-paste of an argument.

jyknight created this revision. The whitespace should come from the argument name in the macro expansion, rather than from the token passed to the macro (same as it does when not pasting). Added a new test case for the change in behavior to stringize_space.c. FileCheck'ized macro_paste_commaext.

[PATCH] D30427: Fix whitespace before token-paste of an argument.

jyknight added a comment. Ping https://reviews.llvm.org/D30427 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30427: Fix whitespace before token-paste of an argument.

This revision was automatically updated to reflect the committed changes. Closed by commit rL302195: Fix whitespace before token-paste of an argument. (authored by jyknight). Changed prior to commit: https://reviews.llvm.org/D30427?vs=89918&id=97881#toc Repository: rL LLVM https://reviews.l

[PATCH] D33020: [Myriad] Pass -Xclang and -mllvm flags to moviCompile

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. Will submit for you since you don't have an svn account. https://reviews.llvm.org/D33020 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D33020: [Myriad] Pass -Xclang and -mllvm flags to moviCompile

This revision was automatically updated to reflect the committed changes. Closed by commit rL302738: [Myriad] Pass -Xclang and -mllvm flags to moviCompile (authored by jyknight). Changed prior to commit: https://reviews.llvm.org/D33020?vs=98369&id=98538#toc Repository: rL LLVM https://revie

[PATCH] D29117: SPARC: allow usage of floating-point registers in inline ASM

This revision was automatically updated to reflect the committed changes. Closed by commit rL302913: [SPARC] Support 'f' and 'e' inline asm constraints. (authored by jyknight). Changed prior to commit: https://reviews.llvm.org/D29117?vs=85708&id=98782#toc Repository: rL LLVM https://reviews

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

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

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

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

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

[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 > https://gcc.gnu.org/bugzilla/show_bug.cgi

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

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight created this revision. jyknight added reviewers: aaron.ballman, rsmith. Herald added subscribers: dexonsmith, arphaman, martong. Herald added a reviewer: shafik. jyknight requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This builtin

[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) durin42 wrote: > jyknight wrote: > > We do need to fallback to

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight updated this revision to Diff 411692. jyknight added a comment. Minor tweaks to comments and docs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120159/new/ https://reviews.llvm.org/D120159 Files: clang/docs/LanguageExtensions.rst cla

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight updated this revision to Diff 411811. jyknight marked 15 inline comments as done. jyknight added a comment. Update per review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120159/new/ https://reviews.llvm.org/D120159 Files: cl

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight added a comment. In D120159#3341224 , @aaron.ballman wrote: > Btw, I think there may be some functionality missing for AST dumping, so I'd > like to see some additional tests for that. I'm not sure what test would actually show a problem (or l

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight marked 8 inline comments as done. jyknight added a comment. In D120159#3349069 , @aaron.ballman wrote: > Ah, okay, that's a good point. I was thinking this would show up in the AST > in places like: > > template > auto func() -> Ty; > >

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

jyknight added a comment. In D119051#3316026 , @dblaikie wrote: > Ah, looks like this is the existing > https://github.com/itanium-cxx-abi/cxx-abi/issues/66 If you're going to change the ABI, you might as well tackle the rest of the differences mention

[PATCH] D120159: [Clang] Implement __builtin_source_location.

jyknight updated this revision to Diff 412087. jyknight marked an inline comment as done. jyknight added a comment. Fix and test __impl lookup within the definition of std::source_location. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120159/new/

[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__", Twine(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__", Twine(TI.

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

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

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

[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 https://re

[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] 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__", Twine(TI.

[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 "", "i,i,~{dirflag},~{f

<    1   2   3   4   5   >