[PATCH] D31885: Remove TBAA information from LValues representing union members

2017-04-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D31885#731306, @hfinkel wrote: > In https://reviews.llvm.org/D31885#730853, @hfinkel wrote: > > > > > > ... > > > > > > >> (And the nonsensicality of the standard very much continues. The code that > >> we've all agreed is obviously being

[PATCH] D32248: CodeGen: Cast alloca to expected address space

2017-04-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1118 +AddrTy->getElementType()->getPointerTo(ExpectedAddrSpace)), +address.getAlignment()); + } This is a lot of work to be doing in a pretty common routine for the benefit of

[PATCH] D32401: [Devirtualization] insert placement new barrier with -O0

2017-04-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Won't you have the exact same problem when LTO'ing with code that wasn't compiled with strict v-table pointers? https://reviews.llvm.org/D32401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32401: [Devirtualization] insert placement new barrier with -O0

2017-04-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I continue to be really uncomfortable with the entire design of this optimization, which appears to miscompile code by default, but as long as nobody's suggesting that we actually turn it on by default, I guess it can be your little research-compiler playground. It

[PATCH] D30809: [coroutines] Add codegen for await and yield expressions

2017-03-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCoroutine.cpp:32 struct CGCoroData { + AwaitKind CurrentAwaitKind = AwaitKind::Init; GorNishanov wrote: > majnemer wrote: > > Shouldn't this struct be in an anonymous namespace? > It is forward

[PATCH] D31673: Allow casting C pointers declared using extern "C" to ObjC pointer types

2017-04-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaExprObjC.cpp:3358 var && - var->getStorageClass() == SC_Extern && + !var->isThisDeclarationADefinition() && var->getType().isConstQualified()) { ahatanak wrote: >

[PATCH] D31830: Emit invariant.group.barrier when using union field

2017-04-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),

[PATCH] D32133: CodeGen: Let byval parameter use alloca address space

2017-04-17 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D32133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31830: Emit invariant.group.barrier when using union field

2017-04-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),

[PATCH] D32110: Emit invariant.group loads with empty group md

2017-04-17 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. Repository: rL LLVM https://reviews.llvm.org/D32110 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32187: [CodeGen][ObjC]

2017-04-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I wouldn't mind unconditionally banning this in JumpDiagnostics, actually. https://reviews.llvm.org/D32187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32187: [CodeGen][ObjC]

2017-04-18 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, this looks great. A few tweaks about the diagnostic and LGTM. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4985 +def

[PATCH] D31885: Remove TBAA information from LValues representing union members

2017-04-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks for CC'ing me. There are two problems here. The first is that vectors are aggregates which contain values of their element type. (And honestly, we may need to be more permissive than this because people are pretty lax about the element type in a vector until

[PATCH] D32248: CodeGen: Cast alloca to expected address space

2017-04-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1105-1119 + // Alloca always returns a pointer in alloca address space, which may + // be different from the type defined by the language. For example, + // in C++ the auto variables are in the default address

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-07-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D28691#823810, @Anastasia wrote: > In https://reviews.llvm.org/D28691#820684, @rjmccall wrote: > > > In https://reviews.llvm.org/D28691#820641, @b-sumner wrote: > > > > > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > > > > > >

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-07-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:1 +//===--- SyncScope.h - atomic synchronization scopes *- C++ -*-===// +// Capitalization. Comment at: include/clang/Basic/SyncScope.h:20 + +namespace

[PATCH] D24461: CodeGen: Cast llvm.flt.rounds result to match __builtin_flt_rounds

2017-08-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Did this ever get committed? https://reviews.llvm.org/D24461 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32199: [TySan] A Type Sanitizer (Clang)

2017-08-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Has this proposal run aground? I'm going back over some old patches that I've been CC'ed on and trying to make sure they're not blocking on my review. https://reviews.llvm.org/D32199 ___ cfe-commits mailing list

[PATCH] D35056: GCC ABI incompatibility when passing object with trivial copy ctor, trivial dtor, and non-trivial move ctor

2017-08-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCXXABI.cpp:43 if (RD->hasNonTrivialDestructor()) return false; v.g.vassilev wrote: > rjmccall wrote: > > Does canPassInRegisters() not subsume all of these earlier checks? > No, if I remove them

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-08-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The patch generally looks good, but if you need to handle non-constant scopes, you should submit a new patch to address that. Comment at: lib/CodeGen/CGAtomic.cpp:896 +return V; + auto DestAS =

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-08-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. LGTM. I'm fine with the plan to handle potentially non-constant scopes in a follow-up patch. Comment at: include/clang/Basic/SyncScope.h:21 +/// \brief Defines the synch scope values used by the atomic builtins and +/// expressions +enum class

[PATCH] D32199: [TySan] A Type Sanitizer (Clang)

2017-08-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Looks fine to me. https://reviews.llvm.org/D32199 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

2017-08-15 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. Thanks, that looks great. https://reviews.llvm.org/D36580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

2017-08-16 Thread John McCall via Phabricator via cfe-commits
rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. Class methods can be inherited; this entire approach is bogus. Repository: rL LLVM https://reviews.llvm.org/D36790 ___

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

2017-08-16 Thread John McCall via Phabricator via cfe-commits
rjmccall resigned from this revision. rjmccall added a comment. I don't think I'm best-equipped to review this, sorry. https://reviews.llvm.org/D36527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

2017-08-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, , ]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

2017-08-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:47 + return Scopes; +} + You could just return an ArrayRef to a static const array. Comment at: lib/CodeGen/CGAtomic.cpp:687 + + auto *SC =

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

2017-08-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, , ]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

2017-08-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaLambda.cpp:959 + ? diag::ext_equals_this_lambda_capture_cxx2a + : diag::warn_cxx1z_compat_equals_this_lambda_capture); hamzasood wrote: > faisalv wrote: >

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

2017-08-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:92 +/// \brief Defines the synch scope ABI for OpenCL. +class AtomicScopeOpenCLABI : public AtomicScopeABI { +public: yaxunl wrote: > rjmccall wrote: > > yaxunl wrote: > > > rjmccall

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

2017-08-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:92 +/// \brief Defines the synch scope ABI for OpenCL. +class AtomicScopeOpenCLABI : public AtomicScopeABI { +public: yaxunl wrote: > rjmccall wrote: > > Please correct me if I'm wrong,

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

2017-08-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, , ]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

2017-08-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, , ]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

2017-08-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:46 + Scopes.push_back(SyncScope::OpenCLSubGroup); + return Scopes; +} t-tye wrote: > Should there be an assert/static_assert in case SyncScope enum grows due to > other languages? It

[PATCH] D32210: [Sema][ObjC] Add support for attribute "noescape"

2017-08-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:14024 + + if (OldFT->hasExtParameterInfos()) +for (unsigned I = 0, E = OldFT->getNumParams(); I != E; ++I) ahatanak wrote: > rjmccall wrote: > > I think we generally encourage the uses of

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

2017-08-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGAtomic.cpp:696 +if (S != Default) + SI->addCase(Builder.getInt32(static_cast(S)), B); + t-tye wrote: > rjmccall wrote: > > t-tye wrote: > > > Is it documented in the SyncScope enum that the

[PATCH] D36327: [OpenCL] Allow targets emit optimized pipe functions for power of 2 type sizes

2017-08-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Could you just implement this in SimplifyLibCalls? I assume there's some way to fill in TargetLibraryInfo appropriately for a platform. Is that too late for your linking requirements? https://reviews.llvm.org/D36327

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

2017-08-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:59 +return "opencl_subgroup"; + } +} yaxunl wrote: > rjmccall wrote: > > t-tye wrote: > > > Should there be a default/assert/static_assert to allow SyncScope enum to > > > grow due

[PATCH] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

2017-08-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I think it's fair to treat instancetype as an inherited requirement — that is, the rules of covariant override always apply, which essentially means that overriders of instance-returning methods must also return instancetype whether they say so explicitly or not. But

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

2017-08-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, , ]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

2017-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall closed this revision. rjmccall added a comment. Committed. https://reviews.llvm.org/D35180 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.

2017-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. To me, features that only serve to help compiler development need to meet a higher bar than this. This just seems really marginal. Like Alex said, you should be able to pretty easily write a debugger script that breaks when it sees a specific diagnostic, or a

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

2017-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3832 + "indirect-arg-temp"); +IRCallArgs[FirstIRArg] = CastToAllocaAddrSpace(Addr.getPointer()); Isn't the original code here correct? You're

[PATCH] D35438: CodeGen: Insert addr space cast for automatic/temp var at right position

2017-07-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:76 auto DestAddrSpace = getContext().getTargetAddressSpace(LangAS::Default); +EnsureInsertPoint(); +auto *CurBB = Builder.GetInsertBlock(); IRBuilder already has saveIP() and

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

2017-07-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Hmm. Maybe it would make more sense to allow it to return null, and then add a comment explaining that it will do so if it can't lower the function type yet. https://reviews.llvm.org/D35180 ___ cfe-commits mailing list

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

2017-07-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Exposing these operations seems fine, but I'm not thrilled about the APIs. Of course, the APIs are exactly derived from IRGen's internal APIs, but we'd like to eventually clean those APIs up, and we certainly don't need to emulate them. Comment at:

[PATCH] D35056: GCC ABI incompatibility when passing object with trivial copy ctor, trivial dtor, and non-trivial move ctor

2017-07-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thank you, I like this approach much better, and the IRGen changes seem fine to me. I'd like to defer to someone else (probably Richard) to review whether the changes to completeDefinition() are correct; I'm not up-to-date with how we handle lazy declarations.

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

2017-07-09 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks great, thanks! https://reviews.llvm.org/D35180 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33842: CodeGen: Fix address space of global variable

2017-07-09 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Great, thanks! LGTM. https://reviews.llvm.org/D33842 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

2017-07-11 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. Still looks good to me. Do you need help committing? https://reviews.llvm.org/D35180 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.

2017-07-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. This is a cute hack, but... I'm not sure I accept the premise that it's a noteworthy obstacle to Clang development to do two greps instead of one. And I don't think I've ever had to debug a diagnostic where __FILE__ and __LINE__ information would've been helpful.

[PATCH] D35438: CodeGen: Ensure there is basic block when performing address space cast

2017-07-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Hmm. It doesn't seem unreasonable for this to require an insertion point as a precondition. In what situation do we emit addrspace casts after a return? https://reviews.llvm.org/D35438 ___ cfe-commits mailing list

[PATCH] D35438: CodeGen: Ensure there is basic block when performing address space cast

2017-07-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Oh, of course. Sadly, in C/C++ the declaration point of a variable does not necessarily dominate all uses of the variable, so you need to emit the cast immediately after the alloca. Just temporarily move CGF.Builder to that point; hopefully we can assume that this

[PATCH] D32210: [Sema][ObjC] Add support for attribute "noescape"

2017-07-15 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D32210#810292, @ahatanak wrote: > Address review comments. > > - Allow attaching "noescape" to pointers other than block pointers. Update > the docs accordingly. > - Attach attribute "nocapture" to parameters that are annotated with >

[PATCH] D34444: Teach codegen to work in incremental processing mode.

2017-07-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D3#795175, @v.g.vassilev wrote: > @rjmccall, thanks for the prompt and thorough reply. > > In https://reviews.llvm.org/D3#793311, @rjmccall wrote: > > > Okay. In that case, I see two problems, one major and one potentially > >

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

2017-07-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:1191 +def MipsLongCall : InheritableAttr, TargetSpecificAttr { + let Spellings = [GCC<"long_call">, GCC<"far">, GCC<"near">]; Because this is used for all three attributes, I think you

[PATCH] D35438: CodeGen: Insert addr space cast for automatic/temp var at right position

2017-07-17 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D35438 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

2017-07-20 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Hmm. Could you invert those conditions so that they early-return, just for consistency? Sorry this is dragging out so long, and thanks for being so patient. Comment at: lib/CodeGen/TargetInfo.cpp:2357 +return; +

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

2017-07-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. One minor revision, but otherwise looks great, thank you. Comment at: lib/CodeGen/CodeGenModule.cpp:1166 +if (!IsForDefinition) + getTargetCodeGenInfo().setTargetAttributes(FD, F, *this); + } I think you should probably pass

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

2017-07-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:1910 +const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule , +ForDefinition_t IsForDefinition) const { if (const FunctionDecl *FD = dyn_cast_or_null(D)) { To preserve

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

2017-07-20 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that looks great! Repository: rL LLVM https://reviews.llvm.org/D35479 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35693: [Driver][Darwin] Pass -munwind-table when !UseSjLjExceptions

2017-07-20 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Does the ARM64 ABI call for unwind tables to be emitted for all functions, like the x86-64 ABI does? Anyway, it seems pretty unfortunate that the default behavior breaks exceptions. https://reviews.llvm.org/D35693 ___

[PATCH] D34444: Teach codegen to work in incremental processing mode.

2017-07-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D3#812836, @v.g.vassilev wrote: > In https://reviews.llvm.org/D3#812418, @rjmccall wrote: > > > In https://reviews.llvm.org/D3#795175, @v.g.vassilev wrote: > > > > > @rjmccall, thanks for the prompt and thorough reply. > > > > > >

[PATCH] D31372: Support Microsoft mangling of swift calling convention methods

2017-07-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. This revision now requires changes to proceed. You can't just change the top-level mangling of the symbol because this is used as part of the function type mangling, and those can appear at more-or-less arbitrary positions. I cannot possibly imagine Microsoft actually

[PATCH] D32210: [Sema][ObjC] Add support for attribute "noescape"

2017-07-25 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D32210#819577, @ahatanak wrote: > In https://reviews.llvm.org/D32210#810508, @rjmccall wrote: > > > Hmm. Unfortunately, I'm not sure that's valid. The retains and releases > > of block captures don't protect against anything related to

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-07-25 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D28691#820489, @yaxunl wrote: > In https://reviews.llvm.org/D28691#820466, @b-sumner wrote: > > > Can we drop the "opencl" part of the name and use something like > > __scoped_atomic_*? Also, it may not make sense to support non-constant

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-07-25 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > There are other languages for heterogeneous compute that have scopes, > although not exposed quite as explicitly as OpenCL. For example AMD's "HC" > language. And any language making use of clang and

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-07-25 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D28691#820641, @b-sumner wrote: > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > > > In https://reviews.llvm.org/D28691#820541, @b-sumner wrote: > > > > > There are other languages for heterogeneous compute that have scopes, >

[PATCH] D35056: GCC ABI incompatibility when passing object with trivial copy ctor, trivial dtor, and non-trivial move ctor

2017-07-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCXXABI.cpp:43 if (RD->hasNonTrivialDestructor()) return false; Does canPassInRegisters() not subsume all of these earlier checks? https://reviews.llvm.org/D35056

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

2017-07-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/AttrDocs.td:1336 +if code compiled using ``-mlong-calls`` switch, it forces compiler to use +the ``jal`` instruction to call the function. + }]; sdardis wrote: > rjmccall wrote: > > I suggest the

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

2017-07-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/AttrDocs.td:1336 +if code compiled using ``-mlong-calls`` switch, it forces compiler to use +the ``jal`` instruction to call the function. + }]; sdardis wrote: > rjmccall wrote: > > sdardis wrote:

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

2017-07-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. A few more minor tweaks. Comment at: lib/CodeGen/TargetInfo.cpp:2357 +return; + X86_32TargetCodeGenInfo::setTargetAttributes(D, GV, CGM, IsForDefinition); That function has its own early exit, so do the early exit after calling

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

2017-07-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:2356 + X86_32TargetCodeGenInfo::setTargetAttributes(D, GV, CGM, IsForDefinition); addStackProbeSizeTargetAttribute(D, GV, CGM); No, sorry, I must not have been clear. We still need a

[PATCH] D34444: Teach codegen to work in incremental processing mode.

2017-07-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D3#818047, @v.g.vassilev wrote: > >> I am open to changing this code as well. That should probably be another > >> review. > > > > I agree. Are you comfortable with blocking this review until that lands? > > It seems like it would

[PATCH] D35268: [ObjC] Ambiguous property synthesis should pick the 'readwrite' property and check for incompatible attributes

2017-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This all seems reasonable. LGTM. Repository: rL LLVM https://reviews.llvm.org/D35268 ___ cfe-commits mailing list

[PATCH] D34777: CodeGen: Fix invalid bitcast for coerced function argument

2017-06-29 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D34777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34665: [CodeGen] Fix assertion failure in EmitCallArg

2017-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This seems fine, thanks. https://reviews.llvm.org/D34665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34995: [AMDGPU] Fix size and alignment of size_t and pointer types

2017-07-05 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D34995 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34444: Teach codegen to work in incremental processing mode.

2017-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. What's the relationship between these llvm::Modules that you want to generate? Are you imagining them as separate translation units, or are the subsequent modules more like addenda to the original? Repository: rL LLVM https://reviews.llvm.org/D3

[PATCH] D34444: Teach codegen to work in incremental processing mode.

2017-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Okay. In that case, I see two problems, one major and one potentially major. The major problem is that, as Richard alludes to, you need to make sure you emit any on-demand definitions that Sema registered with CodeGen during the initial CGM's lifetime but used in

[PATCH] D33842: CodeGen: Fix address space of global variable

2017-07-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Looks great, thanks! Just a few minor changes. Comment at: lib/CodeGen/CGBlocks.cpp:1144 + ? CGM.getNSConcreteGlobalBlock() + : CGM.getNullPointer(CGM.VoidPtrPtrTy, +

[PATCH] D35056: GCC ABI incompatibility when passing object with trivial copy ctor, trivial dtor, and non-trivial move ctor

2017-07-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. If this is a common algorithm across all ABIs, can we just put Sema / the AST in charge of computing it? It's not a cheap thing to recompute retroactively, especially for an imported type, and it seems like it's easily derived from the things that go into

[PATCH] D32401: [Devirtualization] insert placement new barrier with -O0

2017-04-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D32401#735127, @Prazek wrote: > In https://reviews.llvm.org/D32401#734921, @rjmccall wrote: > > > I continue to be really uncomfortable with the entire design of this > > optimization, which appears to miscompile code by default, but as long

[PATCH] D32248: CodeGen: Cast alloca to expected address space

2017-04-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1105-1119 + // Alloca always returns a pointer in alloca address space, which may + // be different from the type defined by the language. For example, + // in C++ the auto variables are in the default address

[PATCH] D32601: [CodeGen][ObjC] Don't retain/release capture objects at block creation that are const-qualified

2017-04-27 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I agree, this seems reasonable. LGTM. https://reviews.llvm.org/D32601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D36347: Add new script to launch lldb and set breakpoints for diagnostics all diagnostics seen.

2017-08-04 Thread John McCall via Phabricator via cfe-commits
rjmccall edited reviewers, added: jasonmolenda, spyffe; removed: rjmccall. rjmccall added a comment. Since this is fundamentally an LLDB script, I've tagged a couple of LLDB people to review it. Jason, Sean: the idea here is to make it easier for clang developers to debug unexpected

[PATCH] D32210: [Sema][ObjC] Add support for attribute "noescape"

2017-08-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/AttrDocs.td:138 +Additionally, for block pointers, the same restriction apply to copies of +blocks. For example: + ``` Additionally, when the parameter is a `block pointer

[PATCH] D36437: [clang] Get rid of "%T" expansions

2017-08-07 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Seems reasonable to me. Repository: rL LLVM https://reviews.llvm.org/D36437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D36501: add flag to undo ABI change in r310401

2017-08-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Yeah, I think having an internal C++ ABI version makes a lot more sense than having a million different flags. Is there a reason to expose this as a knob to users at all? Repository: rL LLVM https://reviews.llvm.org/D36501

[PATCH] D35693: [Driver][Darwin] Pass -munwind-table when !UseSjLjExceptions

2017-07-28 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. Sounds fine to me. https://reviews.llvm.org/D35693 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

2017-08-04 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Still LGTM. https://reviews.llvm.org/D28691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35056: GCC ABI incompatibility when passing object with trivial copy ctor, trivial dtor, and non-trivial move ctor

2017-08-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:5742 +// effect of performing a trivial copy of the type. +bool Sema::CanPassInRegisters(CXXRecordDecl *D) { + if (D->isDependentType()) This should probably be called something like

[PATCH] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

2017-08-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added a reviewer: doug.gregor. rjmccall added a comment. Tagging Doug. Repository: rL LLVM https://reviews.llvm.org/D36790 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

2017-08-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprComplex.cpp:163 +else + CGF.EmitLValue(ME->getBase()); +return *Constant; rjmccall wrote: > There's an EmitIgnoredExpr you could use. > > Also, I think it would be fine to

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

2017-08-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11846 // an address space. if (T.getAddressSpace() != 0) { // OpenCL allows function arguments declared to be an array of a type yaxunl wrote: > Anastasia wrote: > > yaxunl wrote: > > >

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

2017-08-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprComplex.cpp:163 +else + CGF.EmitLValue(ME->getBase()); +return *Constant; There's an EmitIgnoredExpr you could use. Also, I think it would be fine to add a generic

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

2017-08-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Well, we don't currently have a concept of a non-canonical qualifier, but I think it probably wouldn't be difficult to support; you would just need ASTContext::getQualifiedType to be aware of it. https://reviews.llvm.org/D35082

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

2017-08-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The meaning we've agreed on for LangAS::Default is to be the address space of local declarations, which corresponds quite well to __private in OpenCL. I think your concern about diagnostics is better addressed by changing the pretty-printer than by changing Sema to

[PATCH] D32248: CodeGen: Cast alloca to expected address space

2017-05-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7291 + Address adjustAddrSpaceForAutoVar(Address A, const VarDecl *VD, + CodeGen::CodeGenFunction ) const override; }; yaxunl wrote: > rjmccall wrote: > > yaxunl wrote: > > > rjmccall

[PATCH] D32977: [OpenCL] Emit function-scope variable in constant address space as static variable

2017-05-10 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks! https://reviews.llvm.org/D32977 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

<    1   2   3   4   5   6   7   8   9   10   >