[PATCH] D57664: [opaque pointer types] Fix the CallInfo passed to EmitCall in some edge cases.

jyknight created this revision. jyknight added reviewers: dblaikie, rsmith. Herald added a subscriber: Anastasia. Herald added a project: clang. Currently, EmitCall emits a call instruction with a function type derived from the pointee-type of the callee. This *should* be the same as the type

[PATCH] D57668: [opaque pointer types] Pass function types for runtime function calls.

jyknight marked an inline comment as done. jyknight added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1028-1030 +llvm::Constant *getPropertyFn = cast( +CGM.getObjCRuntime().GetPropertyGetFunction().getCallee()); if (!getPropertyFn) {

[PATCH] D57664: [opaque pointer types] Fix the CallInfo passed to EmitCall in some edge cases.

jyknight marked an inline comment as done. jyknight added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:3837 +// having pointee types). +llvm::FunctionType *IRFuncTyFromInfo = getTypes().GetFunctionType(CallInfo); +assert(IRFuncTy == IRFuncTyFromInfo);

[PATCH] D57664: [opaque pointer types] Fix the CallInfo passed to EmitCall in some edge cases.

This revision was automatically updated to reflect the committed changes. Closed by commit rC353181: [opaque pointer types] Fix the CallInfo passed to EmitCall in some (authored by jyknight, committed by ). Changed prior to commit: https://reviews.llvm.org/D57664?vs=184975=185313#toc

[PATCH] D53199: Fix the behavior of clang's -w flag.

jyknight added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53199/new/ https://reviews.llvm.org/D53199 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57767: [opaque pointer types] Cleanup CGBuilder's Create*GEP.

jyknight created this revision. jyknight added a reviewer: dblaikie. Herald added subscribers: cfe-commits, jfb, jholewinski. Herald added a project: clang. The various EltSize, Offset, DataLayout, and StructLayout arguments are all computable from the Address's element type and the DataLayout

[PATCH] D62035: [AST] const-ify ObjC inherited class search

jyknight added a comment. I don't really have much to say about this, and the patch is probably fine, but I do note that most of the other accessors on this class also return mutable objects. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62035/new/

[PATCH] D60748: Fix i386 struct and union parameter alignment

jyknight added a comment. I don't think this was correct (where by "correct", there, I mean "what GCC does", as this patch is intended to match GCC behavior). I think this change may well break more cases than it fixes, so IMO, this should be reverted, until it's implemented properly.

[PATCH] D62268: Add back --sysroot support for darwin header search.

jyknight added a comment. In D62268#1512672 , @ldionne wrote: > This LGTM. > > When I did the refactor, all the code was only using `-isysroot` (and never > accessing `--sysroot`), so I thought only `-isysroot` was relevant on Darwin. > Seems like I was

[PATCH] D62268: Add back --sysroot support for darwin header search.

This revision was automatically updated to reflect the committed changes. Closed by commit rC361429: Add back --sysroot support for darwin header search. (authored by jyknight, committed by ). Changed prior to commit: https://reviews.llvm.org/D62268?vs=200813=200815#toc Repository: rC Clang

[PATCH] D62268: Add back --sysroot support for darwin header search.

jyknight created this revision. jyknight added reviewers: ldionne, jfb. Herald added subscribers: cfe-commits, dexonsmith. Herald added a project: clang. Before e97b5f5cf37e ([clang][Darwin] Refactor header search path logic

[PATCH] D59711: PR41183: Don't emit Wstrict-prototypes warning for an implicit function declaration.

This revision was automatically updated to reflect the committed changes. Closed by commit rC360084: PR41183: Dont emit strict-prototypes warning for an implicit function (authored by jyknight, committed by ). Changed prior to commit: https://reviews.llvm.org/D59711?vs=191924=198341#toc

[PATCH] D59711: PR41183: Don't emit Wstrict-prototypes warning for an implicit function declaration.

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

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

jyknight added a comment. There shouldn't be an empty string ("") in the asm output -- that should be a reference to the "l_yes" label, not the empty string. That seems very weird... Even odder: running clang -S on the above without -fno-integrated-as emits a ".quad .Ltmp00" (note the extra

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

jyknight added inline comments. Comment at: lld/ELF/InputFiles.cpp:402 + if (Config->DepLibs) +if (fs::exists(Specifier)) + Driver->addFile(Specifier, /*WithLOption=*/false); bd1976llvm wrote: > jyknight wrote: > > bd1976llvm wrote: > > > jyknight

[PATCH] D59711: PR41183: Don't emit Wstrict-prototypes warning for an implicit function declaration.

jyknight added a comment. Herald added a subscriber: dexonsmith. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59711/new/ https://reviews.llvm.org/D59711 ___ cfe-commits mailing list

[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

[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

[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

[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

[PATCH] D64793: [Driver] Properly use values-X[ca].o, values-xpg[46].o on Solaris

jyknight added a comment. > I fear it is necessary: at least it matches documented behaviour of both the > Sun/Oracle Studio compilers and gcc. I will defer to your opinion here. But -- one last attempt at dissuading you. :) Is this really doing something _important_, or is it just legacy

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

jyknight added a comment. I'm not sure the history behind why these were added as default-on warningsthey don't really seem appropriate as default warnings to me, either. But I think someone else probably ought to approve the change. Repository: rC Clang CHANGES SINCE LAST ACTION

[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

[PATCH] D64793: [Driver] Properly use values-X[ca].o, values-xpg[46].o on Solaris

jyknight added a comment. Is this really necessary? Users don't typically pass -std= to the driver for linking anyways (what do you even pass if you've compiled both C and C++ code?) so this seems a rather odd way to control behavior. How about instead just adding "values-xpg6.o"

[PATCH] D64487: [clang, test] Fix Clang :: Headers/max_align.c on 64-bit SPARC

jyknight accepted this revision. jyknight added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64487/new/ https://reviews.llvm.org/D64487 ___

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

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

[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

[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

[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

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

[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

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

[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

[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

[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

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

[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

[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

[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

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

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

[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

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

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

[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

[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

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

[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

[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

[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

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

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

[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

[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

[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

[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

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

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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

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

[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

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

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

[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

[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

[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

[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

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

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

[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

[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

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

<    1   2   3   4   5   >