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

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

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

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

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

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

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

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

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

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

[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] 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] 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] 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] 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] 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] 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] 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. > 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. 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] 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] 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 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] 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] 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); 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 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] 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 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] 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] 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] D71726: Let clang atomic builtins fetch add/sub support floating point types

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

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

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

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

jyknight added a comment. In D71726#2165445 , @yaxunl wrote: > In D71726#2165424 , @jyknight wrote: > > > Why not have clang always emit atomicrmw for floats, and let > > AtomicExpandPass handle legalizing that int

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

jyknight added a comment. Why not have clang always emit atomicrmw for floats, and let AtomicExpandPass handle legalizing that into integer atomics if necessary, rather than adding a target hook in clang? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71726/new/ https://reviews.llvm.or

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[PATCH] D80417: Fix Darwin 'constinit thread_local' variables.

This revision was automatically updated to reflect the committed changes. jyknight marked an inline comment as done. Closed by commit rGaca3d067efe1: Fix Darwin 'constinit thread_local' variables. (authored by jyknight). Changed prior to commit: https://reviews.llvm.org/D80417?vs=265642&id=2665

[PATCH] D80417: Fix Darwin 'constinit thread_local' variables.

jyknight created this revision. jyknight added a reviewer: rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. Unlike other platforms using ItaniumCXXABI, Darwin does not allow the creation of a thread-wrapper function for a variable in the TU of users. Because of this

[PATCH] D80225: [Driver] Recognize -fuse-ld={bfd, gold, lld} but don't prepend "ld." or "ld64." for other values

jyknight added a comment. It's worrying to me that there number of places in LLVM that at the exact argument value of "-fuse-ld=". E.g. in the windows and PS4 toolchains. We already claim to support arbitrary values and full paths, but if you specify "-fuse-ld=/path/to/lld-link" on Windows toda

[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

jyknight added a comment. In the commit message, you refer to the preferred alignemtn as the "true" alignment, but that's misleading. As discussed on the mailing list thread, it's not true alignment at all. Comment at: clang/include/clang/AST/RecordLayout.h:74 + /// The maxi

[PATCH] D79511: [ObjC] Add compatibility mode for type checking of qualified id block parameters.

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. It looks like you didn't squash your two commits before uploading, so the diff for review now only includes the changes for the comment, not the complete patch. Other than needing to squas

[PATCH] D79511: [ObjC] Add compatibility mode for type checking of qualified id block parameters.

jyknight added inline comments. Comment at: clang/test/SemaObjC/block-type-safety.m:170 +genericBlockWithParam = blockWithParam; +blockWithParam = genericBlockWithParam; // expected-error {{incompatible block pointer types assigning to 'void (^)(NSAllArray *)' from 'void

[PATCH] D66831: [ObjC] Fix type checking for qualified id block parameters.

jyknight added a comment. In D66831#2019125 , @vsapsai wrote: > Agree that is a mistake in `NSItemProvider` API. The solution I offer is not > to revert the change but to add cc1 flag > `-fcompatibility-qualified-id-block-type-checking` and to make the d

[PATCH] D66831: [ObjC] Fix type checking for qualified id block parameters.

jyknight added a comment. Hi, it seems that this change is incompatible with the `[NSItemProvider loadItemForTypeIdentifier:options:completionHandler:]` method's expected (but extremely weird and unusual!) usage. Per https://developer.apple.com/documentation/foundation/nsitemprovider/1403900-l

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

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. 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. 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. 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. 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] 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] 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] 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] 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] 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. In D67983#1723681 , @rjmccall wrote: > We could probably do a quick check to see if the class informally conforms to > the protocol. `+copyWithZone` seems to be marked unavailable in ARC; not > sure if that would cause problems

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

jyknight added a comment. In D67983#1723376 , @thakis wrote: > After this, Class can no longer be used as a key type in an Obj-C dictionary > literal. Is that intentional? Such code was already an on by default incompatible-pointer-types warning in Obj

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGccc4d83cda16: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer… (authored by jyknight). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D67982: [ObjC] Add some additional test cases around pointer conversions.

This revision was automatically updated to reflect the committed changes. Closed by commit rG1c982af05997: [ObjC] Add some additional test cases around pointer conversions. (authored by jyknight). Changed prior to commit: https://reviews.llvm.org/D67982?vs=221586&id=225439#toc Repository: rG

[PATCH] D28213: [Frontend] Correct values of ATOMIC_*_LOCK_FREE to match builtin

jyknight reopened this revision. jyknight added a comment. This revision is now accepted and ready to land. The close was due to phabricator problem, reopening. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D28213/new/ https://reviews.llvm.org/D2821

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

jyknight added a comment. The `abort()` function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior? Either `_exit()` (long available extension, which lld already uses) or `quick_exit()` (the new C standard way) seem possibly preferable?

[PATCH] D68410: [AttrDocs] document always_inline

jyknight added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4399-4400 + let Content = [{ +Hint that inline substitution should be attempted when optimizations are +disabled. Does not guarantee that inline substitution actually occurs. +}];

[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] D67982: [ObjC] Add some additional test cases around pointer conversions.

jyknight added a comment. (See https://reviews.llvm.org/D67983 for the proposed behavior change.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67982/new/ https://reviews.llvm.org/D67982 ___ cfe-commit

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

jyknight added a comment. Note that the test-case diffs are on top of https://reviews.llvm.org/D67982, which I split out to make the actual change in behavior in this commit clearer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67983/new/ https:/

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

jyknight created this revision. jyknight added a reviewer: rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. For example, in Objective-C mode, the initialization of 'x' in: @implementation MyType + (void)someClassMethod { MyType *x = self; } @end is cor

[PATCH] D67982: [ObjC] Add some additional test cases around pointer conversions.

jyknight created this revision. jyknight added a reviewer: rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. This is especially important for Objective-C++, which is entirely missing this testing at the moment. This annotates with "FIXME" the cases which I change in

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

jyknight created this revision. jyknight added a reviewer: rsmith. Herald added subscribers: cfe-commits, jfb, kristof.beyls. Herald added a project: clang. It is specified to return a bool in GCC's documentation and implementation, but the clang builtin says it returns an int. This would be prett

[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] 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] 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] D65582: IR: accept and print numbered %N names for function args

jyknight added a comment. Herald added a subscriber: wuzish. +1 for doing this. I started looking at fixing this when I modified the printer to print proper labels for numbered basic-blocks (instead of comments), but I didn't do so because of the amount of test churn was off-putting. I think th

<    1   2   3   4   5   >