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

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:1910 +const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule &CGM, +ForDefinition_t IsForDefinition) const { if (const FunctionDecl *FD = dyn_cast_or_null(D)) { To preserve ex

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

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

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 fo

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

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

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 ch

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

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; + X86_32TargetCodeGenInfo::setTar

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

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 http

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

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

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

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] D34444: Teach codegen to work in incremental processing mode.

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 sig

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

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"

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 esca

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

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

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 targe

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

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

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] D40023: [RISCV] Implement ABI lowering

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH asb wrote: > rjmccall wrote: > > I feel like a better design would be to record whether to do a sext or a >

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:3529 + /// Basic properties of non-trivial C structs. + bool HasStrongObjCPointer : 1; + Is it interesting to track all the individual reasons that a struct is non-trivial at the struct level

[PATCH] D41999: Refactor handling of signext/zeroext in ABIArgInfo

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that looks great. I appreciate you doing this. Repository: rC Clang https://reviews.llvm.org/D41999 ___ cfe-commits mailing list

[PATCH] D40023: [RISCV] Implement ABI lowering

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH asb wrote: > rjmccall wrote: > > asb wrote: > > > rjmccall wrote: > > > > I feel like a better design would

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1148 +DK_objc_weak_lifetime, +DK_c_struct_strong_field }; ahatanak wrote: > rjmccall wrote: > > I don't think you want to refer to the fact that the C struct specifically > > contain

[PATCH] D40023: [RISCV] Implement ABI lowering

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. LGTM, but you should wait for Eli's sign-off, too. https://reviews.llvm.org/D40023 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D40569: Use default IR alignment for cleanup.dest.slot.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. r322406 Repository: rC Clang https://reviews.llvm.org/D40569 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:239 + bit IncludeC = includeC; +} aaron.ballman wrote: > rjmccall wrote: > > aaron.ballman wrote: > > > rjmccall wrote: > > > > aaron.ballman wrote: > > > > > rjmccall wrote: > > > > > > I

[PATCH] D41311: [CodeGen] Fix crash when a function taking transparent union is redeclared.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D41311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall accepted this revision. rjmccall added a comment. Looks great to me! Thanks for taking this on, it's a pretty major improvement for users. Would you like to create an issue with itanium-cxx-abi to document this, or do you want me to handle that? https://reviews.llvm.org/D41039 __

[PATCH] D42154: Don't generate inline atomics for i386/i486

rjmccall added a comment. In https://reviews.llvm.org/D42154#977991, @wmi wrote: > In https://reviews.llvm.org/D42154#977975, @efriedma wrote: > > > The LLVM backend currently assumes every CPU is Pentium-compatible. If > > we're going to change that in clang, we should probably fix the backend

[PATCH] D41539: [CodeGen] Decorate aggregate accesses with TBAA tags

rjmccall added a comment. Thank you. Maybe there should be an overload of EmitAggregateCopy that takes LValues? A lot of these cases start with an LValue on at least one side, and there are already some convenience functions to turn an Address into a naturally-typed LValue. https://reviews.

[PATCH] D42092: implement C++ dr388 for the Itanium C++ ABI: proper handling of catching exceptions by reference

rjmccall added a comment. This is definitely something that the personality function should handle. If we want to do heroic things in the absence of personality function support, we can, but the code should at least be written to be conditional on personality support. If we can rev the person

[PATCH] D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenTBAA.h:67 /* BaseType= */ nullptr, /* AccessType= */ nullptr, - /* Offset= */ 0, /* Size= */ 0); + /* Offset= */ 0, /* Size= */ UINT64_MAX);

[PATCH] D42154: Don't generate inline atomics for i386/i486

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Well, my point is that the example in the linked bug is asking for 486 code-generation, which is apparently unsupported by LLVM. Anyway, it's not a good reason to hold up this patch, since

[PATCH] D41539: [CodeGen] Decorate aggregate accesses with TBAA tags

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, that works, thanks! LGTM. https://reviews.llvm.org/D41539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.or

[PATCH] D42508: AST: support protocol conformances on id/class/interfaces in MS ABI

rjmccall added a comment. The basic idea here seems fine to me; I'll leave David to review the details. Repository: rC Clang https://reviews.llvm.org/D42508 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D42521: [CodeGen] Use the non-virtual alignment when emitting the base class subobject constructor

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D42521 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

rjmccall added inline comments. Comment at: lib/AST/ExprClassification.cpp:652 +if (Ctx.getCanonicalType(ASE->getBase()->getType()).isConstQualified()) + return Cl::CM_ConstQualified; + Is there a reason why the places that compute the type of these l-va

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

rjmccall added inline comments. Comment at: lib/AST/ExprClassification.cpp:652 +if (Ctx.getCanonicalType(ASE->getBase()->getType()).isConstQualified()) + return Cl::CM_ConstQualified; + avt77 wrote: > rjmccall wrote: > > Is there a reason why the places

[PATCH] D41677: Change memcpy/memove/memset to have dest and source alignment attributes.

rjmccall accepted this revision. rjmccall added a comment. Still LGTM. Repository: rC Clang https://reviews.llvm.org/D41677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42614: AST: support ObjC lifetime qualifiers in MS ABI

rjmccall added a comment. It's not just that functions can't be overloaded on the parameter-variable type qualifier — it's not part of the function type at all, just like making a parameter 'const int' instead of 'int' is not part of the function type. I understand that MSVC mangles some thing

[PATCH] D42614: AST: support ObjC lifetime qualifiers in MS ABI

rjmccall added a comment. In https://reviews.llvm.org/D42614#990808, @compnerd wrote: > Hmm, the only thing that we could fake would be namespaces on the parameter > types. Is that better? I'm not tied to re-using the existing mangling. I'm just worried about re-using the existing mangling c

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

rjmccall added inline comments. Comment at: lib/Parse/ParseObjc.cpp:1434 MaybeParseGNUAttributes(paramAttrs); - ArgInfo.ArgAttrs = paramAttrs.getList(); } aaron.ballman wrote: > rjmccall wrote: > > ObjC parameter syntax is really its own weird

[PATCH] D42660: [PR32482] Fix bitfield layout for -mms-bitfield and pragma pack

rjmccall added inline comments. Comment at: lib/AST/RecordLayoutBuilder.cpp:1564 unsigned MaxFieldAlignmentInBits = Context.toBits(MaxFieldAlignment); + unsigned RecordFieldAlign = FieldAlign; if (!MaxFieldAlignment.isZero() && FieldSize) { I think naming

[PATCH] D42660: [PR32482] Fix bitfield layout for -mms-bitfield and pragma pack

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Oh, that makes much more sense, thanks. Repository: rL LLVM https://reviews.llvm.org/D42660 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D42768: AST: add an extension to support SwiftCC on MS ABI

rjmccall added a comment. Is demangling "correctly" really a more important goal here than not spuriously failing when presented with a swiftcall function type in a non-top-level position? I don't know that there was anything wrong with the previous patch's approach to this if we're just going

[PATCH] D42768: AST: add an extension to support SwiftCC on MS ABI

rjmccall added a comment. No, I mean things like `void foo(__attribute__((swiftcall)) void (*fnptr)());`. Repository: rC Clang https://reviews.llvm.org/D42768 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/m

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

rjmccall added a comment. That's still just const-propagation. The const qualifier on the pointee type of this should propagate to the l-value resulting from the member access, and the vector-specific projection logic should continue to propagate const to the type of the element l-value. htt

[PATCH] D42811: [CodeGen][va_args] Correct Vector Struct va-arg 'in_reg' code gen

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. LGTM. Repository: rC Clang https://reviews.llvm.org/D42811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

rjmccall added a comment. If you just want a better diagnostic, there's quite a bit of machinery already set up to give more specific errors about why such-and-such l-value isn't modifiable. There may even be vector-specific diagnostics for that already, just triggered from the wrong place in

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

rjmccall added a comment. No, really, in CreateBuiltinArraySubscriptExpr and LookupMemberExpr, in the clauses where they handle vector types, you just need to propagate qualifiers from the base type like is done in BuildFieldReferenceExpr. There's even a FIXME about it in the former. https:/

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

rjmccall added a comment. Hmm. Was there discussion about that? I see no reason not to bump ObjC++. https://reviews.llvm.org/D29739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

rjmccall added a comment. In https://reviews.llvm.org/D29739#673933, @tigerleapgorge wrote: > Hi John, > > Here is the most recent discussion I can find on cfe-dev. > “I'm guessing that Objective-C/C++ is kind of passe, so nobody is really > interested in modernizing it” > http://lists.llvm.or

[PATCH] D29859: Make Lit tests C++11 compatible - nounwind noexcept

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D29859 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

rjmccall added a comment. In https://reviews.llvm.org/D29739#674288, @probinson wrote: > In https://reviews.llvm.org/D29739#673971, @rjmccall wrote: > > > In https://reviews.llvm.org/D29739#673933, @tigerleapgorge wrote: > > > > > Hi John, > > > > > > Here is the most recent discussion I can find

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

rjmccall added a comment. In https://reviews.llvm.org/D29739#675277, @probinson wrote: > In https://reviews.llvm.org/D29739#674297, @rjmccall wrote: > > > In https://reviews.llvm.org/D29739#674288, @probinson wrote: > > > > > I really think Apple would need to step up here if the default > > > O

[PATCH] D24812: Lit C++11 Compatibility Patch #11

rjmccall added a reviewer: rnk. rjmccall added a comment. Generally looks good to me, thanks. One question for Reid. Comment at: test/CodeGenCXX/static-init.cpp:14 +// CHECK98: @_ZZN5test414useStaticLocalEvE3obj = linkonce_odr global %"struct.test4::HasVTable" zeroinitializer

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D29739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D29908: Disallow returning a __block variable via a move

rjmccall added a comment. In https://reviews.llvm.org/D29908#676601, @ahatanak wrote: > In https://reviews.llvm.org/D29908#676589, @ahatanak wrote: > > > Posting John's review comment, which was sent to the review I abandoned: > > > > "Hmm. The behavior I think we actually want here is that we s

[PATCH] D24812: Lit C++11 Compatibility Patch #11

rjmccall added inline comments. Comment at: test/CodeGenCXX/static-init.cpp:14 +// CHECK98: @_ZZN5test414useStaticLocalEvE3obj = linkonce_odr global %"struct.test4::HasVTable" zeroinitializer, comdat, align 8 +// CHECK11: @_ZZN5test414useStaticLocalEvE3obj = linkonce_odr global

[PATCH] D24812: Lit C++11 Compatibility Patch #11

rjmccall added a comment. In https://reviews.llvm.org/D24812#678931, @tigerleapgorge wrote: > @rjmccall > > Hi John, I've made the changes to volatile.cpp. > I take it this patch is good for commit? Yes, I think I have the information I wanted from Reid. LGTM. https://reviews.llvm.org/D2481

[PATCH] D29986: Fix crash when an incorrect redeclaration only differs in __unaligned type-qualifier

rjmccall added a comment. LGTM. https://reviews.llvm.org/D29986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29986: Fix crash when an incorrect redeclaration only differs in __unaligned type-qualifier

rjmccall added a comment. In https://reviews.llvm.org/D29986#684811, @rogfer01 wrote: > Ping? Are you expecting something even more conclusive than "looks good to me"? Because I sent you that feedback a week ago. :) https://reviews.llvm.org/D29986

[PATCH] D30345: [CodeGen][Blocks] Refactor capture handling in code that generates block copy/destroy routines

rjmccall added a comment. You're doing this refactor to... support doing another refactor of the same code? Why are these patches separate? Repository: rL LLVM https://reviews.llvm.org/D30345 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D30430: Make Lit tests C++11 compatible - IR ordering

rjmccall added a comment. The C++98 behavior here is not really vital to test precisely; it's just minor differences in what gets instantiated and when. I think it's fine to just update the run line to -std=c++11 for things like this. But if you really want to test both configurations, this L

[PATCH] D25556: [Sema] Add variable captured by a block to the enclosing lambda's potential capture list

rjmccall added a comment. Well, I was hoping that Richard would weigh in, but absent that, I don't have any objections. Since it fixes a known bug, let's just go ahead and do it. LGTM. https://reviews.llvm.org/D25556 ___ cfe-commits mailing list

[PATCH] D30345: [CodeGen][Blocks] Refactor capture handling in code that generates block copy/destroy routines

rjmccall added a comment. In https://reviews.llvm.org/D30345#688298, @arphaman wrote: > In https://reviews.llvm.org/D30345#688144, @rjmccall wrote: > > > You're doing this refactor to... support doing another refactor of the same > > code? Why are these patches separate? > > > Not quite, by "me

[PATCH] D30345: [CodeGen][Blocks] Refactor capture handling in code that generates block copy/destroy routines

rjmccall added a comment. Thanks, LGTM. Repository: rL LLVM https://reviews.llvm.org/D30345 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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 declare

[PATCH] D27157: IRGen: Remove all uses of CreateDefaultAlignedLoad.

rjmccall added a comment. Thanks for doing this! A couple minor questions / comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2195 LoadInst *Load = -Builder.CreateDefaultAlignedLoad(IntToPtr, /*isVolatile=*/true); +Builder.CreateAlignedLoad(IntTy, IntT

[PATCH] D27157: IRGen: Remove all uses of CreateDefaultAlignedLoad.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuilder.h:126 // FIXME: these "default-aligned" APIs should be removed, // but I don't feel like fixing all the builtin code right now. llvm::StoreInst *CreateDefaultAlignedStore(llvm::Value *Val, --

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

rjmccall added inline comments. Comment at: test/CodeGenCXX/return.cpp:21 + // CHECK-NOSTRICT-NEXT: load + // CHECK-NOSTRICT-NEXT: ret i32 + // CHECK-NOSTRICT-NEXT: } mehdi_amini wrote: > Quuxplusone wrote: > > Can you explain why a load from an uninitialized

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

rjmccall added a comment. In https://reviews.llvm.org/D27163#607100, @rsmith wrote: > In https://reviews.llvm.org/D27163#607078, @rsmith wrote: > > > A target-specific default for this, simply because there's a lot of code on > > Darwin that happens to violate this language rule, doesn't make se

[PATCH] D27157: IRGen: Remove all uses of CreateDefaultAlignedLoad.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuilder.h:126 // FIXME: these "default-aligned" APIs should be removed, // but I don't feel like fixing all the builtin code right now. llvm::StoreInst *CreateDefaultAlignedStore(llvm::Value *Val, --

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:306 + /// \param AddrSpace address space of pointee in source language. + virtual uint64_t getNullPtrValue(unsigned AddrSpace) const { +return 0; We normally spell out "Pointer", I

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

rjmccall added inline comments. Comment at: include/clang/AST/APValue.h:379 void setLValue(LValueBase B, const CharUnits &O, NoLValuePath, - unsigned CallIndex); + unsigned CallIndex, bool IsNuppPtr); void setLValue(LValueBase B, const CharUn

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1532 + return llvm::ConstantInt::get(ConvertType(DestTy), + CGF.getContext().getTargetNullPtrValue(E->getType())); assert(!DestTy->isBooleanType() && "bool should use PointerToBool");

[PATCH] D22296: CodeGen: New vtable group representation: struct of vtable arrays.

rjmccall added a comment. This seems reasonable to me. Just a few representational / API requests. Comment at: clang/include/clang/AST/VTableBuilder.h:222 + typedef llvm::DenseMap> + AddressPointsMapTy; Please use a struct instead of std::pair. =

[PATCH] D24431: CodeGen: Start using inrange annotations on vtable getelementptr.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I'm not sure I like this IR design, but this use of it seems fine. LGTM. https://reviews.llvm.org/D24431 ___ cfe-commits mailing list cfe-co

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1532 + return llvm::ConstantInt::get(ConvertType(DestTy), + CGF.getContext().getTargetNullPtrValue(E->getType())); assert(!DestTy->isBooleanType() && "bool should use PointerToBool");

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. With that resolved, this LGTM. https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D22296: CodeGen: New vtable group representation: struct of vtable arrays.

rjmccall added a comment. A couple minor suggestions, then LGTM. Comment at: clang/include/clang/AST/VTableBuilder.h:255 +operator ArrayRef() const { return {data(), size()}; }; + }; + Maybe this ought to be in LLVM as OwnedArrayRef? And the more minimal

[PATCH] D27627: Allow target to specify default address space for codegen

rjmccall added a comment. What address space should local variables be in? https://reviews.llvm.org/D27627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27627: Allow target to specify default address space for codegen

rjmccall added a comment. Ok, I'm not understanding this, then. I declare a local variable: int x; According to you, x should be in the generic address space, i.e. the private address space. And at the LLVM IR level, this will be implemented with an AllocaInst, which always creates memory

[PATCH] D27627: Allow target to specify default address space for codegen

rjmccall added a comment. I suspect that what you actually want to do in order to implement HCC is change SemaType so that 'int*' is implicitly interpreted as 'int __something *'. But I don't know how not having explicit address spaces actually works in the HCC language design, given that it's

[PATCH] D27627: Allow target to specify default address space for codegen

rjmccall added a comment. Because, notably, if you do that, then an attempt to pass &x as an int* will fail, which means this isn't really C++ anymore... and yet that appears to be exactly what you want. https://reviews.llvm.org/D27627 ___ cfe-com

[PATCH] D22296: CodeGen: New vtable group representation: struct of vtable arrays.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks! https://reviews.llvm.org/D22296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D27627: Allow target to specify default address space for codegen

rjmccall added a comment. In https://reviews.llvm.org/D27627#619928, @yaxunl wrote: > > Because, notably, if you do that, then an attempt to pass &x as an int* > > will fail, which means this isn't really C++ anymore... and yet that > > appears to be exactly what you want. > > How about when ge

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#730539, @dberlin wrote: > In https://reviews.llvm.org/D31885#730072, @rjmccall wrote: > > > Thanks for CC'ing me. There are two problems here. > > > > The second is that our alias-analysis philosophy says that the fact that > > two ac

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

rjmccall added a comment. The rule we actually want is that no reference to the block derived from the parameter value will survive after the function returns. You can include examples of things that would cause potential escapes, but trying to actually lock down the list seems ill-advised. A

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#730799, @dberlin wrote: > In https://reviews.llvm.org/D31885#730766, @rjmccall wrote: > > > In https://reviews.llvm.org/D31885#730539, @dberlin wrote: > > > > > In https://reviews.llvm.org/D31885#730072, @rjmccall wrote: > > > > > > > T

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#730853, @hfinkel wrote: > > There was a deliberate decision to make TBAA conservative about type > > punning in LLVM because, in practice, the strict form of TBAA you think we > > should follow has proven to be a failed optimization p

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#730920, @dberlin wrote: > Just so i understand: Ignoring everything else (we can't actually make > likelyalias work, i think the code in the bugs makes that very clear), None of the code in the bugs I've seen makes that very clear, b

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#730958, @dberlin wrote: > In https://reviews.llvm.org/D31885#730936, @rjmccall wrote: > > > In https://reviews.llvm.org/D31885#730920, @dberlin wrote: > > > > > Just so i understand: Ignoring everything else (we can't actually make > >

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

rjmccall added a comment. In https://reviews.llvm.org/D32210#731192, @ahatanak wrote: > In https://reviews.llvm.org/D32210#730777, @rjmccall wrote: > > > The rule we actually want is that no reference to the block derived from > > the parameter value will survive after the function returns. You

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

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 m

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

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 on

[PATCH] D32199: [TBAASan] A TBAA Sanitizer (Clang)

rjmccall added a comment. If you're going to try to enforce the declared type of memory, you'll also need something like the C effective type rule to handle char buffers in C++. As far as I can tell, it's not actually legal under the spec to cast an array of chars to an arbitrary type and acce

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

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 http://lists.llvm.org/

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

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 mi

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

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] D32248: CodeGen: Cast alloca to expected address space

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] D32401: [Devirtualization] insert placement new barrier with -O0

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

<    25   26   27   28   29   30   31   32   33   >