[PATCH] D49952: Check for NULL Destination-Type when creating ArrayConstant
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Sure, that's fine. https://reviews.llvm.org/D49952 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added a comment. I would expect this to replace the existing warning, not to appear together with it. Repository: rC Clang https://reviews.llvm.org/D50278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added inline comments.
Comment at: test/Sema/conditional-expr.c:78
+ // expected-error@-1{{converting
'__attribute__((address_space(2))) int *' to type 'void *' changes address
space of pointer}}
+ // expected-error@-2{{converting
'__attribute__((address_space(3))) int *' to type 'void *' changes address
space of pointer}}
Also, these diagnostics seem wrong. Where is `void *` coming from?
Repository:
rC Clang
https://reviews.llvm.org/D50278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1630 +if (const CXXDestructorDecl *DD = RD->getDestructor()) + if (const auto FPT = DD->getType()->getAs()) +// Conservatively assume the destructor can throw if the exception I'm pretty sure this step can't fail. Comment at: lib/CodeGen/CGBlocks.cpp:1643 +if (Ctx.getBlockVarCopyInits(VD)) + return true; + return false; Can you just ask Sema to check `canThrow` for the expression and pass it down? Repository: rC Clang https://reviews.llvm.org/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added inline comments.
Comment at: test/Sema/conditional-expr.c:78
+ // expected-error@-1{{converting
'__attribute__((address_space(2))) int *' to type 'void *' changes address
space of pointer}}
+ // expected-error@-2{{converting
'__attribute__((address_space(3))) int *' to type 'void *' changes address
space of pointer}}
leonardchan wrote:
> rjmccall wrote:
> > Also, these diagnostics seem wrong. Where is `void *` coming from?
> When dumping the AST this is what the resulting type is for the conditional
> expression already is if the operands are 2 pointers with different address
> spaces.
>
> According to this comment, the reason seems to be because this is what GCC
> does:
>
> ```
> 6512 // In this situation, we assume void* type. No especially good
> 6513 // reason, but this is what gcc does, and we do have to pick
> 6514 // to get a consistent AST.
> ```
That makes sense in general, but in this case it's not a great diagnostic; we
should just emit an error when trying to pick a common type.
Repository:
rC Clang
https://reviews.llvm.org/D50278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1643 +if (Ctx.getBlockVarCopyInits(VD)) + return true; + return false; ahatanak wrote: > ahatanak wrote: > > rjmccall wrote: > > > Can you just ask Sema to check `canThrow` for the expression and pass it > > > down? > > Since this changes the existing behavior, I made changes to > > test/CodeGenCXX/block-byref-cxx-objc.cpp to test it. Previously, IRGen > > would emit an invoke to call `_Block_object_assign` when the constructor > > was marked as noexcept. > Perhaps I misunderstood your comment, should I have Sema set a flag or > something in Expr when it calls a function that can throw? Sema has a `canThrow` predicate that it uses when checking things like the `noexcept` expression. I was thinking that you could pass that down with the copy expression in the AST for the block capture. Constructors can have default-argument expressions that can throw even if the constructor itself can't, so it's important to do it that way. Repository: rC Clang https://reviews.llvm.org/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added a comment. You might want to ask Richard on IRC if there are caveats when using that for these purposes. Repository: rC Clang https://reviews.llvm.org/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:6522 +bool HasDifferingLAddrSpace = LAddrSpace != ResultAddrSpace; +bool HasDifferingRAddrSpace = RAddrSpace != ResultAddrSpace; + I was going to tell you to use the predicate `Qualifiers::isAddressSpaceSupersetOf` here, but then I was looking at the uses of that, and I think the real fix is to just go into the implementation of `checkConditionalPointerCompatibility` and make the compatibility logic not OpenCL-specific. The fast-path should just be whether the address spaces are different. And it looks like this function has a bug where it always uses `LangAS::Default` outside of OpenCL even if the pointers are in the same address space. Repository: rC Clang https://reviews.llvm.org/D50278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added a comment. That is a change that Richard should definitely sign off on. Also, I'm not sure this works — is it really okay to skip the work done by `ResolveExceptionSpec` in IRGen? What does that mean, that we're just somewhat more conservative than we would otherwise be? And why is this a better solution than just storing whether the copy-expression throws in `BlockDecl::Capture`? Repository: rC Clang https://reviews.llvm.org/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall added a comment. In https://reviews.llvm.org/D50278#1190544, @ebevhan wrote: > I think the solution to a lot of diagnostic and behavior issues with address > spaces is to remove predication on OpenCL. It's a bit odd to have a language > feature that is enabled out of the box regardless of langoptions (address > spaces) but won't actually work properly unless you're building OpenCL. I agree; almost all of the address-space-related restrictions predicated on OpenCL are actually general restrictions that apply across all address-space extensions. OpenCL's rules only differ from what's laid out in TR 18037 in how certain declarations default to specific address spaces. It's unfortunate that the code reviewers at the time (which probably included me at least a little) didn't try harder to push for the more general rule. As it is, it would be a good project for someone who's working on address spaces a lot to just audit all the OpenCL-specific checks in Sema to see which of them should be generalized. Repository: rC Clang https://reviews.llvm.org/D50278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/Sema/SemaExpr.cpp:6522 +bool HasDifferingLAddrSpace = LAddrSpace != ResultAddrSpace; +bool HasDifferingRAddrSpace = RAddrSpace != ResultAddrSpace; + leonardchan wrote: > rjmccall wrote: > > I was going to tell you to use the predicate > > `Qualifiers::isAddressSpaceSupersetOf` here, but then I was looking at the > > uses of that, and I think the real fix is to just go into the > > implementation of `checkConditionalPointerCompatibility` and make the > > compatibility logic not OpenCL-specific. The fast-path should just be > > whether the address spaces are different. > > > > And it looks like this function has a bug where it always uses > > `LangAS::Default` outside of OpenCL even if the pointers are in the same > > address space. > I'm not sure how the `LangAS::Default`, but removing all checks for OpenCL > does the trick and prints an existing error relating to different > address_spaces on conditional operands to replace the warning. Only 2 tests > needed the change from the expected warning to expected error without having > to change any OpenCL tests. > > I also think the address_space comparison is already done with the > `lhQual.isAddressSpaceSupersetOf` and `rhQual.isAddressSpaceSupersetOf`. Er, you're right, of course, since `isAddressSpaceSupersetOf` is a non-strict ordering. If that operation ever gets big enough that we don't want to inline the whole thing, we can at least make sure the fast-path is inlinable and then outline the complicated stuff. We can also worry about that later. Repository: rC Clang https://reviews.llvm.org/D50278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added a comment. In https://reviews.llvm.org/D50152#1191777, @ahatanak wrote: > Since BlockVarCopyInits is a map with key `VarDecl *`, I think we want to add > a flag to VarDecl (NonParmVarDeclBits) that indicates whether the copy > expression can throw or not. Or is there a reason to store the bit in > `BlockDecl::Capture` instead? I was thinking about the non-`__block` capture case, sorry. For `__block` variables, I think the values of that map should just be... well, I'd make a struct wrapping it, but basically a `PointerIntPair` of an `Expr*` and the `canThrow` flag. Repository: rC Clang https://reviews.llvm.org/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.
rjmccall added inline comments.
Comment at: lib/AST/MicrosoftMangle.cpp:448
mangleVariableEncoding(VD);
- else
+ else if (!isa(D))
llvm_unreachable("Tried to mangle unexpected NamedDecl!");
I don't think we want `ObjCInterfaceDecl`s to enter this function normally; I'd
prefer to leave that assertion intact.
Comment at: lib/AST/MicrosoftMangle.cpp:2574
mangleTagTypeKind(TTK_Struct);
- mangleName(T->getDecl());
+ mangle(T->getDecl(), ".objc_cls_");
}
You're not really reusing any interesting logic from `mangle` here; please just
stream the literal directly into `Out`.
Also, this will add the actual identifier to the substitution set, whereas your
implementation below in the case for ObjCObjectType adds the prefixed
identifier to the substitution set.
Comment at: lib/CodeGen/CGBlocks.cpp:1262
+ if (IsWindows) {
+auto *Init = llvm::Function::Create(llvm::FunctionType::get(CGM.VoidTy,
+ {}), llvm::GlobalValue::InternalLinkage, ".block_isa_init",
theraven wrote:
> DHowett-MSFT wrote:
> > Is there value in emitting a list of blocks that need to be initialized,
> > then initializing them in one go in a COMDAT-foldable function?
> I think that the best solution is to move this into the back end, so that
> this code goes away in the front end, but anything that's referring to a
> dllimport global in a global initialiser is transformed in the back end to a
> list of initialisations and a comdat function that walks the list and sets
> them up. That said, this seems sufficiently generally useful that it would
> be nice for the function to be in the CRT bits.
>
>
> I should be in Redmond some time in October, so maybe we can discuss it with
> some of the VS team then?
Can the blocks part of this patch be split out? It's basically a totally
different bugfix.
Comment at: lib/CodeGen/CGBlocks.cpp:1216
bool IsOpenCL = CGM.getLangOpts().OpenCL;
+ bool IsWindows = CGM.getTarget().getTriple().isOSWindows();
if (!IsOpenCL) {
Should this be a PE/COFF-specific restriction? Or otherwise somehow restricted
to the "native" environment? I don't know enough about all the UNIX-on-Windows
approaches to know whether the same restriction would apply to all of them. I
did think they generally used the Itanium C++ ABI, and the Itanium ABI relies
on linking v-tables from the C++ standard library. Maybe those environments
just all use static linking to get around that.
Comment at: lib/CodeGen/CGBlocks.cpp:1276
+InitVar->setSection(".CRT$XCLa");
+CGM.addUsedGlobal(InitVar);
+ }
Is the priority system not good enough?
Comment at: lib/CodeGen/CGObjCGNU.cpp:915
+return name;
+ }
/// The GCC ABI superclass message lookup function. Takes a pointer to a
Can this section-names cleanup also just be in a separate patch?
Comment at: lib/CodeGen/CGObjCGNU.cpp:2262
llvm::Constant *CGObjCGNUstep::GetEHType(QualType T) {
+ if (CGM.getContext().getTargetInfo().getTriple().isWindowsMSVCEnvironment())
+return CGM.getCXXABI().getAddrOfRTTIDescriptor(T);
I think this is the third different way that you test for Windows in this
patch. :) Is there a reason why this is just testing for MSVC and the others
are testing for PE/COFF or for Windows generally?
Comment at: lib/CodeGen/CGObjCGNU.cpp:3817
+ if (isRethrow && CGF.CGM.getTarget().getTriple().isWindowsMSVCEnvironment())
{
+CGF.EmitRuntimeCallOrInvoke(ExceptionReThrowFn).setDoesNotReturn();
+ }
You're sure here that the static information aligns with the dynamic?
Comment at: lib/CodeGen/CGObjCRuntime.cpp:125
llvm::Constant *TypeInfo;
+unsigned Flags;
};
Please add some sort of comment about the meaning and source of these flags.
Comment at: lib/CodeGen/EHScopeStack.h:424
+ void Emit(CodeGenFunction &CGF, Flags F) override;
+};
+
Please add a function on SGF to push this cleanup or something instead of
globally declaring it.
Repository:
rC Clang
https://reviews.llvm.org/D50144
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1276
+InitVar->setSection(".CRT$XCLa");
+CGM.addUsedGlobal(InitVar);
+ }
DHowett-MSFT wrote:
> rjmccall wrote:
> > Is the priority system not good enough?
> My reading of the LLVM language reference leads me to believe it’s only
> ordered per-module. If that’s the case, the benefit of emitting into `XC*` is
> that it provides guaranteed order over all linker input.
>
> `llvm.global_ctors` excerpt:
>
> > The functions referenced by this array will be called in ascending order of
> > priority (i.e. lowest first) when the module is loaded.
>
> Now if the priority system _is_ guaranteed over all linker input, will that
> guarantee hold for mixed Clang and CL objects?
Init priorities on ELF are preserved in the section name like
`.init_array.200`, and the sections get sorted by that in the image — it's
really a very similar trick to how it works with `.CRT$XC`. Of the major
formats, I think it's just Mach-O that doesn't have any built-in prioritization
mechanism across object files. But I don't know if LLVM actually tries to
translate init priorities over into `.CRT$XC` suffices when targeting PE/COFF,
and arguably that's good: init priorities as presented in LLVM right now are
pretty specific to the ELF mechanism. Long-term, maybe `llvm.global_ctors`
should be generalized so that on ELF targets it takes an integer priority, on
PE/COFF targets it takes a string, and on Mach-O it doesn't take anything at
all; but I won't hold up this patch for that.
On the other hand, I tend to agree that maybe the best solution is for the
backend to just take care of this and automatically create a global initializer
to install non-function (or maybe non-function & `unnamed_addr`) `dllimport`ed
symbols in global data.
Repository:
rC Clang
https://reviews.llvm.org/D50144
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50436: Correctly initialise global blocks on Windows.
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, LGTM, at least until we have that backend support you mentioned. Repository: rC Clang https://reviews.llvm.org/D50436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.
rjmccall added inline comments.
Comment at: lib/CodeGen/CGObjCGNU.cpp:3542
+ allSelectors.push_back(entry.first);
+std::sort(allSelectors.begin(), allSelectors.end());
mgrang wrote:
> Please use llvm::sort instead of std::sort. See
> https://llvm.org/docs/CodingStandards.html#beware-of-non-deterministic-sorting-order-of-equal-elements.
Also, why is the sort necessary? Should this change be separately committed
and tested?
Comment at: lib/CodeGen/CGObjCGNU.cpp:915
+return name;
+ }
/// The GCC ABI superclass message lookup function. Takes a pointer to a
theraven wrote:
> rjmccall wrote:
> > Can this section-names cleanup also just be in a separate patch?
> This is difficult to extract, because it's mostly needed for the COFF part
> where we need to modify the section names. For ELF, it was fine to keep them
> as separate `const char*`s
I don't mind if the extracted patch seems unmotivated on its own, but I do
think it should be a separate patch.
Comment at: lib/CodeGen/CGObjCGNU.cpp:2262
llvm::Constant *CGObjCGNUstep::GetEHType(QualType T) {
+ if (CGM.getContext().getTargetInfo().getTriple().isWindowsMSVCEnvironment())
+return CGM.getCXXABI().getAddrOfRTTIDescriptor(T);
theraven wrote:
> rjmccall wrote:
> > I think this is the third different way that you test for Windows in this
> > patch. :) Is there a reason why this is just testing for MSVC and the
> > others are testing for PE/COFF or for Windows generally?
> For the EH logic, we are using DWARF exceptions if we are targeting a Windows
> environment that uses DWARF EH and SEH-based exceptions if we are targeting a
> Windows environment that uses MSVC-compatible SEH.
>
> The section code is specific to PE/COFF, where we don't get to use some of
> the ELF tricks.
>
> The blocks code is specific to Windows, because it relates to Windows
> run-time linking. Hypothetically, a PE/COFF platform could provide dynamic
> relocations that would eliminate the need for that check.
>
> It's possible that some of the PE/COFF vs Windows distinctions are wrong.
> For example, UEFI images are PE/COFF binaries and if anyone wanted to use
> blocks in a UEFI firmware image then they may find that they need the Windows
> code path (but I do not have a good way of testing this, so restricted it to
> Windows initially). Similarly, some of the section initialisation code in
> CGObjCGNU may actually be Windows-only, but it's probably a good starting
> point for anyone who actually wants to use Objective-C in UEFI firmware
> (though the final destination for such people is likely to involve padded
> cells).
Okay, the exception logic makes sense. Please make this a common predicate
instead of repeating it in different places.
My understanding is that the restriction on the use of `dllimport`-ed symbols
is a general PE restriction: the format itself only supports binds in the
import address table, and everything else has just to be a rebase. I mean, of
course you can imagine a loader that extends PE to support arbitrary binds, but
you can also imagine a loader that extends PE to support just embedding an ELF
image. Firmware probably never uses imported symbols.
Comment at: lib/CodeGen/CGObjCGNU.cpp:3817
+ if (isRethrow && CGF.CGM.getTarget().getTriple().isWindowsMSVCEnvironment())
{
+CGF.EmitRuntimeCallOrInvoke(ExceptionReThrowFn).setDoesNotReturn();
+ }
theraven wrote:
> rjmccall wrote:
> > You're sure here that the static information aligns with the dynamic?
> I'm not sure I understand this question.
Your new code path no longer uses `ExceptionAsObject`, which is our static
notion of the current exception value. Instead, you call a runtime function
which presumably relies on a dynamically-stored current exception value. I'm
just asking if, in this lexical position, you're definitely rethrowing the
right exception.
Comment at: lib/CodeGen/CGObjCRuntime.cpp:125
llvm::Constant *TypeInfo;
+unsigned Flags;
};
theraven wrote:
> rjmccall wrote:
> > Please add some sort of comment about the meaning and source of these flags.
> These are not well documented anywhere, including in the Clang Microsoft C++
> ABI code that I read to see why exceptions weren't working, but I've added a
> small comment that explains why they exist. I have no idea where the values
> come from though.
Just mentioning that they're the same catch flags used in the Microsoft C++ ABI
is all I'm asking for. Although maybe there could be an abstraction for that
instead of passing them around separately?
Repository:
rC Clang
https://reviews.llvm.org/D50144
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:248 + /// Mapping from __block VarDecls to their copy initialization expr. The + /// boolean flag indicates whether the expression can throw. + typedef llvm::DenseMaphttps://reviews.llvm.org/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:248 + /// Mapping from __block VarDecls to their copy initialization expr. The + /// boolean flag indicates whether the expression can throw. + typedef llvm::DenseMap Maybe you should just make a type for this pairing. You can put this > documentation there, and the access functions can take and return it. I don't think the `typedef` is needed here. Comment at: include/clang/AST/ASTContext.h:158 +Expr *CopyExpr; +bool CanThrow; + }; Using a PointerIntPair in the implementation of this is still a reasonable choice. Just make it a proper abstraction, with a constructor with the two arguments and getters for the two fields. Comment at: include/clang/AST/ASTContext.h:2680 + /// indicates whether the copy expression can throw or not. + void setBlockVarCopyInits(const VarDecl* VD, Expr *CopyExpr, bool CanThrow); I know this is longstanding, but since you're changing all the call sites anyway, please remove the trailing `s` from these two method names. Comment at: lib/AST/ASTContext.cpp:2511 "getBlockVarCopyInits - not __block var"); - llvm::DenseMap::iterator -I = BlockVarCopyInits.find(VD); - return (I != BlockVarCopyInits.end()) ? I->second : nullptr; + BlockVarCopyInitMap::const_iterator I = BlockVarCopyInits.find(VD); + if (I != BlockVarCopyInits.end()) I think `auto` is okay for things like this. Comment at: lib/CodeGen/CGBlocks.cpp:1682 + if (IsCopyHelper && Ctx.getBlockVarCopyInits(Var).CanThrow) +Name += "c"; +} I don't think you need to add `d` to the name of a copy helper. It's a bit weird, but while copying a `__block` variable can cause its copy helper to run, destroying it immediately afterwards can never cause its destroy helper to run. That's because a newly-copied `__block` variable always has a reference count of 2: the new reference in the copy and the forwarding reference from the original. I think that means you can just add a single letter which specifies whether the corresponding `__block` variable operation is known to be able to throw. Comment at: lib/CodeGen/CGBlocks.cpp:1688 +if (F == BLOCK_FIELD_IS_BLOCK) + Name += "b"; + } Why `rb` for a captured block instead of some single-letter thing? You don't need to emulate the structure of the flags here. Comment at: lib/CodeGen/CGBlocks.cpp:1705 + IsVolatile, Ctx); + Name += llvm::to_string(Str.size()) + "_" + Str; + break; The underscore is necessary here because non-trivial destructor strings can start with a number? Worth a comment. Repository: rC Clang https://reviews.llvm.org/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1682
+ if (IsCopyHelper && Ctx.getBlockVarCopyInits(Var).CanThrow)
+Name += "c";
+}
ahatanak wrote:
> rjmccall wrote:
> > I don't think you need to add `d` to the name of a copy helper. It's a bit
> > weird, but while copying a `__block` variable can cause its copy helper to
> > run, destroying it immediately afterwards can never cause its destroy
> > helper to run. That's because a newly-copied `__block` variable always has
> > a reference count of 2: the new reference in the copy and the forwarding
> > reference from the original.
> >
> > I think that means you can just add a single letter which specifies whether
> > the corresponding `__block` variable operation is known to be able to throw.
> I added 'd' to the name of the copy helper functions only because IRGen
> generates different code depending on whether the destructor can throw or not.
>
> For example, if I compile the following code with -DTHROWS, IRGen uses
> 'invoke' (which jumps to the terminate block) for the calls to
> `_Block_object_dispose` on the EH path whereas it uses 'call' if the
> destructor doesn't throw.
>
> ```
> struct S {
> S();
> #ifdef THROWS
> ~S() noexcept(false);
> #else
> ~S() noexcept(true);
> #endif
> S(const S &);
> int a;
> };
>
> void test() {
> __block S s0, s1, s2;
> ^{ (void)s0, (void)s1; (void)s2; };
> }
> ```
>
> It seems like IRGen doesn't have to use 'invoke' when emitting a call to
> `_Block_object_dispose` even when the class has a destructor that can throw,
> if I understood your explanation correctly?
Right. It's specifically only true when unwinding after a copy, which is very
atypical for C++ code, but nonetheless it's true. We should make the call
`nounwind` in these situations and leave a comment explaining why. Did my
explanation make any sense?
Comment at: lib/CodeGen/CGBlocks.cpp:1688
+if (F == BLOCK_FIELD_IS_BLOCK)
+ Name += "b";
+ }
ahatanak wrote:
> rjmccall wrote:
> > Why `rb` for a captured block instead of some single-letter thing? You
> > don't need to emulate the structure of the flags here.
> I can use a single letter here, but I'm already using 'b' for byref captures.
> Perhaps I can use 'o' for non-arc objects, instead of 'r', and use 'r' for
> byref?
That seems reasonable.
Repository:
rC Clang
https://reviews.llvm.org/D50152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. Herald added a subscriber: jfb. Hey, still working on this? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added inline comments.
Comment at: include/clang/AST/ASTContext.h:161
+Expr *getCopyExpr() const;
+bool canThrow() const;
+llvm::PointerIntPair ExprAndFlag;
These should all just be defined inline.
Comment at: lib/CodeGen/CGBlocks.cpp:1725
BlockFieldFlags Flags, bool EHOnly,
+ bool DisposeCannotThrow, VarDecl *Var,
CodeGenFunction &CGF) {
Could you replace these two flags with something more semantic, like telling
this function what the context of pushing the cleanup is — basically meaning,
are we in the copy helper or the destroy helper? That will let you pull the
comment explaining `DisposeCannotThrow` into this function, where it makes a
lot more sense.
Repository:
rC Clang
https://reviews.llvm.org/D50152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50527: [Parser] Support alternative operator token keyword args in Objective-C++
rjmccall added a comment. Assuming you've done enough source-compatibility testing to say with reasonable confidence that this won't break anything, I think this is fine. It's a core goal of Objective-C/C++ to allow the base language as a complete subset if at all possible. https://reviews.llvm.org/D50527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1725
BlockFieldFlags Flags, bool EHOnly,
+ bool DisposeCannotThrow, VarDecl *Var,
CodeGenFunction &CGF) {
ahatanak wrote:
> rjmccall wrote:
> > Could you replace these two flags with something more semantic, like
> > telling this function what the context of pushing the cleanup is —
> > basically meaning, are we in the copy helper or the destroy helper? That
> > will let you pull the comment explaining `DisposeCannotThrow` into this
> > function, where it makes a lot more sense.
> I thought about passing a single flag instead of passing two flags too. If we
> are going to pass a single flag, should we still use two variables inside the
> function, EHOnly and DisposeCannotThrow, to maintain the readability of the
> code?
Computing `EHOnly` at the top makes sense to me. `DisposeCannotThrow` is
really only interesting in the one case, and the analysis makes more sense in
terms of a `ForCopyHelper` parameter than it would in terms of this more
abstract concept.
Repository:
rC Clang
https://reviews.llvm.org/D50152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Repository: rC Clang https://reviews.llvm.org/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.
rjmccall accepted this revision.
rjmccall added a comment.
LGTM.
Comment at: lib/CodeGen/CGObjCGNU.cpp:915
+return name;
+ }
/// The GCC ABI superclass message lookup function. Takes a pointer to a
theraven wrote:
> rjmccall wrote:
> > theraven wrote:
> > > rjmccall wrote:
> > > > Can this section-names cleanup also just be in a separate patch?
> > > This is difficult to extract, because it's mostly needed for the COFF
> > > part where we need to modify the section names. For ELF, it was fine to
> > > keep them as separate `const char*`s
> > I don't mind if the extracted patch seems unmotivated on its own, but I do
> > think it should be a separate patch.
> None of the changes to do this are in a separate commit, and they touch
> enough of the code that it's very difficult to separate them without ending
> up with conflicts in this branch (which touches the code surrounding the
> changes in multiple places). I cannot extract the code in such a way that
> this patch would cleanly rebase on top, because various things (e.g. the null
> section code) needed restructuring to work with Windows and touch this logic.
Alright, I won't insist.
Comment at: lib/CodeGen/CGObjCGNU.cpp:3817
+ if (isRethrow && CGF.CGM.getTarget().getTriple().isWindowsMSVCEnvironment())
{
+CGF.EmitRuntimeCallOrInvoke(ExceptionReThrowFn).setDoesNotReturn();
+ }
theraven wrote:
> rjmccall wrote:
> > theraven wrote:
> > > rjmccall wrote:
> > > > You're sure here that the static information aligns with the dynamic?
> > > I'm not sure I understand this question.
> > Your new code path no longer uses `ExceptionAsObject`, which is our static
> > notion of the current exception value. Instead, you call a runtime
> > function which presumably relies on a dynamically-stored current exception
> > value. I'm just asking if, in this lexical position, you're definitely
> > rethrowing the right exception.
> Ah, I see. This code path is hit only as a `@throw;`, not a `@throw obj` (in
> the latter case, there is a non-null return from `S.getThrowExpr()`). In
> this case, the value of ExceptionAsObject may not be useful. In the case of a
> catchall, this is a load from a stack location with no corresponding store.
> The Windows unwind logic keeps the object on the stack during funclet
> execution and then continues the unwind at the end, without ever providing
> this frame's funclets with a pointer to it. That's probably not very
> obvious, so I've added an explanatory comment.
Thanks.
Repository:
rC Clang
https://reviews.llvm.org/D50144
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
rjmccall added a comment. You can't test that there's no non-determinism, but you can certainly test that we emit selectors in sorted order as opposed to the order in which they're used. I imagine a function with a bunch of `@selector` expressions should be good enough for that. Repository: rC Clang https://reviews.llvm.org/D50559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast
rjmccall added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1016
+ if (DstScale > SrcScale) {
+// Need to allocate space before shifting left
+ResultWidth = SrcWidth + DstScale - SrcScale;
In IR, this isn't really "allocating" space.
Comment at: lib/CodeGen/CGExprScalar.cpp:1034
+ if (DstFPSema.isSaturated() &&
+ (CGF.getContext().getCorrespondingSaturatedType(SrcTy) != DstTy)) {
+auto Mask = APInt::getBitsSetFrom(
Why is this condition based on the formal types exactly matching rather than
something about the FP semantics being different? Formal types can correspond
to the same format, right?
We need to check for saturation if we're either (1) decreasing the magnitude of
the highest usable bit or (2) going signed->unsigned, (2) we're going
signed->unsigned, or (3) we're going unsigned->signed without increasing the
number of integral bits. And I'd expect the checks we have to do in each case
to be different.
Comment at: lib/Sema/SemaExpr.cpp:5889
+case Type::STK_MemberPointer:
+ llvm_unreachable("Unimplemented conversion from FixedPoint to type");
+}
Is there something I'm missing that actually diagnoses the unimplemented cases
here? There's a lot of code that seems to assume that any two arithmetic types
can be converted to each other, and we do prefer not to crash the compiler,
especially on valid code.
Repository:
rC Clang
https://reviews.llvm.org/D50616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. I appreciate the fact that you spelled it all out in the test, too. LGTM. Comment at: lib/CodeGen/CGObjCGNU.cpp:3547 + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); theraven wrote: > bmwiedemann wrote: > > compilation failed here: > > ../tools/clang/lib/CodeGen/CGObjCGNU.cpp:2444:11: error: 'sort' is not a > > member of 'llvm' > > > > it suggested std::sort > I'm not sure `llvm::sort` was part of the 6.0 release, it was added in April > and so should be in 7.0. I don't expect a patch against the 8 branch to > apply cleanly to 6... `array_pod_sort` has been around forever if we need a variant of the patch for that branch. Repository: rC Clang https://reviews.llvm.org/D50559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields
rjmccall added a comment. We should absolutely have static assertions to check that these bit-field types don't get larger than 32 bits. A lot of the subclass layouts have been tweaked to fit that (packing into the tail padding of `Type` on 64-bit targets), so accidentally overflowing to use more bits in the base is going to lead to a lot of bloat. Repository: rC Clang https://reviews.llvm.org/D50631 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type
rjmccall added a comment. Oh, I missed that there was a separate review for this. A lot of the important subclasses that need extra storage have been designed with the expectation that these bit-fields fit within 32 bits. For example, `FunctionType` starts with a bunch of bit-fields because the expectation is that they'll fit into the tail-padding of `Type`. So this assertion should really be testing <= 4, and if we need to land a few patches first to make that true, we should do so. Repository: rC Clang https://reviews.llvm.org/D50630 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21767: Fix instantiation of friend function templates
rjmccall added a comment. Shouldn't there just be a link in the AST from the instantiated `FunctionTemplateDecl ` back to the original pattern? Maybe a generalization of `InstantiatedFromMember` in `RedeclarablableTemplateDecl`? Repository: rC Clang https://reviews.llvm.org/D21767 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type
rjmccall added a comment. In https://reviews.llvm.org/D50630#1197795, @riccibruno wrote: > @rjmccall > > I would argue that we should make these bit-fields take 8 bytes for the > following reasons: > > 1. On 64 bits archs, this is free since we have already a little less than 8 > bytes of padding here, assuming Type keep its 18 bits. First, `Type` is not 16-byte aligned as far as the compiler is concerned; it is merely dynamically aligned to a 16-byte boundary when allocated. You actually verified this when you did your experiment of printing out the sizes of various `Type` subclasses, because the value of `sizeof` is always rounded up to the alignment, meaning that a size of 40 would not be legal if `Type` were actually 16-byte-aligned. Because `Type` is only 4/8-byte aligned, its only tail padding is what's needed to fill up a pointer after these bit-fields, which is supposed to be 4 bytes but is instead apparently 0 bytes because we overflowed one of the bit-fields. Not officially aligning `Type` to 16 bytes is fairly important for memory efficiency even on 64-bit hosts because many of our types use tail-allocated storage, which naturally starts at `sizeof(T)` bytes from the address point; if we formally aligned `Type` subclasses to 16 bytes, we'd often waste a pointer of that storage. Second, tail padding of base classes is not necessarily wasted under the Itanium C++ ABI. Itanium will start placing sub-objects in the tail-padding of the last non-empty base class as long as that base is POD under the rules of C++03; `Type` is not POD because it has a user-provided constructor. Looking at `Type.h`, we don't seem to be taking advantage of this as much as I thought, at least in the `Type` hierarchy (maybe we do in the `Stmt` or `Decl` hierarchies). We have a lot of subclasses that have 32 bits of miscellaneous storage but either (1) don't order their fields correctly to allow subclasses to fit in or (2) also subclass `FoldingSetNode`, which breaks up the optimization. Since we do care primarily about 64-bit hosts, I'm leaning towards agreeing that we should just accept that we have 64 bits of storage here, 46 of which are available to subclasses. If you're interested in optimizing this, it would be nice if you could go through all the subclasses looking for 32-bit chunks that could be profitably moved up into `Type`. > 2. On 32 bits archs, this a priori increase the size of each Type by 4 bytes. > However, types are aligned to 16 bytes because of the fast CVR qualifiers. > Doing an fsyntax-only on all of Boost with -print-stats I get that by far the > most commonly used Types have size 40 or 48 (on a 64 bits arch). That is 5 or > 6 pointers. This would be 20 or 24 bytes on 32 bits archs if we assume that > every member of the most commonly used types are pointers and that we shrink > the bitfields to 32 bits. But since Types are aligned to 16 bytes we do not > gain anything. Types aren't the only thing allocated in the ASTContext, so allocating a 16-byte-aligned chunk with a non-multiple-of-16 size doesn't mean that any difference between the size and the next 16-byte boundary is necessarily wasted. But you're probably right that the difference between `Type` being 12 and 16 bytes is probably not a big deal. John. Repository: rC Clang https://reviews.llvm.org/D50630 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type
rjmccall added a comment. In https://reviews.llvm.org/D50630#1198053, @riccibruno wrote: > I actually did exactly this. My approach was to extend what is already done, > that is add nested classes SomethingBitfields in Type and add an instance of > each to the anonymous union. The classes which I have found so far benefiting > from this are FunctionProtoType, TemplateSpecializationType, > PackExpansionType, > DependentTemplateSpecializationType and SubstTemplateTypeParmPackType. > > For example the patch dealing with TemplateSpecializationType is > here https://reviews.llvm.org/D50643. I see. This is one of those rare cases where your changes probably would've been clearer not broken up into multiple patches. :) I only got CC'ed on two of them. Anyway, I think we're all in agreement that this is the right thing to do now. John. Repository: rC Clang https://reviews.llvm.org/D50630 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type
rjmccall added a comment. Sure, that seems like a reasonable optimization. Repository: rC Clang https://reviews.llvm.org/D50630 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50715: [AST] Store the OwnedTagDecl as a trailing object in ElaboratedType.
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D50715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21767: Fix instantiation of friend function templates
rjmccall added a comment.
Oh, I see. The code currently tries to work with just the specialization and
the pattern. To do the instantiation, we have to find template arguments for
the context in which the pattern appears. For function temploids that aren't
defined in a friend declaration, we can just consider the semantic DC hierarchy
of the specialization, which will be the same across all redeclarations. For
function temploids that *are* defined in a friend declaration, we have to look
at the lexical DC of the declaration that was actually instantiated from the
class temploid that defined the friend function, which isn't necessarily the
specialization because it might have been redeclared after the friend function
was instantiated. So your patch is mostly just changing the find-the-pattern
lookup to remember that lexical DC when we find a pattern this way, which seems
sensible. Richard should look over the actual lookup-algorithm changes.
Comment at: include/clang/AST/Decl.h:2443
+ /// defined in a friend declaration, this parameter is assigned pointer to
the
+ /// class containing the friend declaration.
+ ///
"If this is non-null and the pattern is a friend function declaration, this is
set to the class containing the friend declaration."
Comment at: lib/AST/Decl.cpp:3389
+ if (Orig->getFriendObjectKind() != Decl::FOK_None) {
+if (auto *RDC = dyn_cast(D->getLexicalDeclContext()))
+ Host = RDC;
I don't think this can fail; a friend should always be lexically nested in a
class.
Comment at: lib/AST/Decl.cpp:3399
+return Def;
+ }
+}
It's unfortunate that this needs to walk the redeclaration chain itself looking
for definitions. Can't you just call `getDefinition` to find the definition
and then run your extra checks on that?
Comment at: lib/AST/Decl.cpp:3444
+if (MFD->getFriendObjectKind() != Decl::FOK_None)
+ if (auto *DC = dyn_cast(getLexicalDeclContext()))
+if (HostForFriend)
Same observation.
Repository:
rC Clang
https://reviews.llvm.org/D21767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields
rjmccall added a comment. Our experience is that we keep adding more complexity to `FunctionType`, so it'd be nice if the bits weren't pressed up against the absolute limit. Dynamic exception specifications are really common, but only in the zero-exceptions case, so as long as we can efficiently represent and detect that I think putting the rest of the count out-of-line is fine. Repository: rC Clang https://reviews.llvm.org/D50631 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast
rjmccall added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1042
+std::min(DstScale + DstFPSema.getIntegralBits(), ResultWidth));
+Value *Zero = ConstantInt::get(CGF.getLLVMContext(), APInt(ResultWidth,
0));
+
You can just pass 0 here and it'll be zero-extended to the necessary width.
Comment at: lib/CodeGen/CGExprScalar.cpp:1044
+
+Value *IsNegative = nullptr;
+if (Mask != 0) {
I'm sorry, but this code is really impenetrable. The variable names are
non-descriptive, and there are a lot of uncommented dependencies between
values, like how `IsNegative` propagates out, and like how it's checking
without explanation that there's not a magnitude change using whether the mask
ends up being all-zero. Please just assign the two components of
`ShouldCheckSaturation` to reasonably-named local variables and then use those
to guide the code-generation here.
Also, the code being generated here is pretty weird. I'm not sure the mask is
helping; it might both produce better code and be easier to understand if you
just broke it down into cases, like this:
```
if a magnitude check is required {
auto Max = maximum value of dest type;
auto TooHigh = IsSigned ? Builder.CreateICmpSGT(Result, Max) :
Builder.CreateICmpUGT(Result, Max);
Result = Builder.CreateSelect(TooHigh, Max, Result);
}
if signed -> unsigned {
auto Zero = zero value of dest type;
Result = Builder.CreateSelect(Builder.CreateICmpSLT(Result, Zero), Zero,
Result);
} else if (IsSigned) {
auto Min = minimum value of dest type;
Result = Builder.CreateSelect(Builder.CreateICmpSLT(Result, Min), Min,
Result);
}
```
Comment at: lib/CodeGen/CGExprScalar.cpp:1034
+ if (DstFPSema.isSaturated() &&
+ (CGF.getContext().getCorrespondingSaturatedType(SrcTy) != DstTy)) {
+auto Mask = APInt::getBitsSetFrom(
leonardchan wrote:
> rjmccall wrote:
> > Why is this condition based on the formal types exactly matching rather
> > than something about the FP semantics being different? Formal types can
> > correspond to the same format, right?
> >
> > We need to check for saturation if we're either (1) decreasing the
> > magnitude of the highest usable bit or (2) going signed->unsigned, (2)
> > we're going signed->unsigned, or (3) we're going unsigned->signed without
> > increasing the number of integral bits. And I'd expect the checks we have
> > to do in each case to be different.
> For simplicity, I more or less copied the logic from
> `APFixedPoint::convert()` which performs a saturation check that covers all
> of these cases if the destination semantics were saturated.
>
> Added another condition that checks if we need to perform saturation checks.
> I think your (1) and (3) might be the same thing since I think we only really
> need to check if the magnitude decreases or if going from signed -> unsigned.
>
> I think though that the IR emission would be the same since both cases will
> require checking for a change in the magnitude (via the mask). The only
> difference is that if going from signed->unsigned, the min saturation is zero
> if the value is negative.
Wow, sorry for the edit failure in that review comment. You're right, it
should've been just (1) and the first (2).
Are there no fixed-point formats for which the range doesn't go up to (almost)
1? I guess there probably aren't.
Comment at: lib/Sema/SemaExpr.cpp:5889
+case Type::STK_MemberPointer:
+ llvm_unreachable("Unimplemented conversion from FixedPoint to type");
+}
leonardchan wrote:
> rjmccall wrote:
> > Is there something I'm missing that actually diagnoses the unimplemented
> > cases here? There's a lot of code that seems to assume that any two
> > arithmetic types can be converted to each other, and we do prefer not to
> > crash the compiler, especially on valid code.
> The plan was to implement these in other patches. I wasn't sure if
> `llvm_unreachable()` was ok to use as a placeholder for features that will
> eventually be implemented.
>
> Added diagnostics here for now to be eventually removed once these casts are
> implemented in later patches.
You can still use `llvm_unreachable` for the cases here that aren't arithmetic
types, namely all the pointer types.
Repository:
rC Clang
https://reviews.llvm.org/D50616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50783: [CodeGen] Merge identical block descriptor global variables
rjmccall added a comment. In https://reviews.llvm.org/D50783#1200868, @ahatanak wrote: > A few points I forgot to mention: > > - This optimization kicks in only in NonGC mode. I don't think we need to > care much about GC anymore, so I think that's OK. Yes, that's fine. > - There is a lot of redundancy among the copy/dispose helper function strings > and the block layout string in the block descriptor name (they all encode the > information about the captures), which can make the descriptor name long. If > that becomes a problem, it's possible to encode the information in a way that > shortens the descriptor name, but that would probably make the function that > generates the name more complex. It should be straightforward to at least get merge the copy/dispose helper function names, right? Those make basically the same pass over the captures and just check for slightly different things; the block-descriptor summary just has to include a little of both. To unique the block layout string as well, we'd just have to cover the captures for which we haven't added any other description. If you want to punt on that, I think that's fine. Repository: rC Clang https://reviews.llvm.org/D50783 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50783: [CodeGen] Merge identical block descriptor global variables
rjmccall added a comment. Okay. Repository: rC Clang https://reviews.llvm.org/D50783 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression
rjmccall added inline comments.
Comment at: test/CodeGenObjC/forward-declare-protocol-gnu.m:6
-Protocol *getProtocol(void)
-{
- return @protocol(P);
-}
+@interface I
+@end
arphaman wrote:
> rjmccall wrote:
> > Does this really not require a definition of `P`? Ugh. I wonder if that's
> > reasonable to fix, too.
> Nope, we don't emit the protocol metadata for it. It might make sense to
> require the definition with the implementation?
Yeah, I think so. I would argue that there no places where we should be
emitting metadata for an incomplete protocol.
Comment at: test/Parser/objc-cxx-keyword-identifiers.mm:22
+@protocol P2;
+@protocol delete // expected-error {{expected identifier; 'delete' is a
keyword in Objective-C++}}
+@end
arphaman wrote:
> rjmccall wrote:
> > Why did this test need to change?
> We need to declare `delete` because it's used in a `@protocol` expression on
> line 63.
Ah, gotcha.
Repository:
rC Clang
https://reviews.llvm.org/D49462
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast
rjmccall added a comment. In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote: > In https://reviews.llvm.org/D50616#1202034, @ebevhan wrote: > > > I think I've mentioned this before, but I would like to discuss the > > possibility of adding a target hook(s) for some of these operations > > (conversions, arithmetic when it comes). Precisely matching the emitted > > saturation operation is virtually impossible for a target. > > > Sorry I forgot to address this also. Just to make sure I understand this > correctly since I haven't used these before: target hooks are essentially for > emitting target specific code for some operations right? Does this mean that > the `EmitFixedPointConversion` function should be moved to a virtual method > under `TargetCodeGenInfo` that can be overridden and this is what get's > called instead during conversion? If this is more than just a hobby — like if there's a backend that wants to do serious work with matching fixed-point operations to hardware support — we should just add real LLVM IR support for fixed-point types instead of adding a bunch of frontend customization hooks. I don't know what better opportunity we're expecting might come along to justify that, and I don't think it's some incredibly onerous task to add a new leaf to the LLVM type hierarchy. Honestly, LLVM programmers across the board have become far too accustomed to pushing things opaquely through an uncooperative IR with an obscure muddle of intrinsics. In the meantime, we can continue working on this code. Even if there's eventually real IR support for fixed-point types, this code will still be useful; it'll just become the core of some legalization pass in the generic backend. Repository: rC Clang https://reviews.llvm.org/D50616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast
rjmccall added a comment. In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote: > In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote: > > > Sorry I forgot to address this also. Just to make sure I understand this > > correctly since I haven't used these before: target hooks are essentially > > for emitting target specific code for some operations right? Does this mean > > that the `EmitFixedPointConversion` function should be moved to a virtual > > method under `TargetCodeGenInfo` that can be overridden and this is what > > get's called instead during conversion? > > > Yes, the thought I had was to have a virtual function in TargetCodeGenInfo > that would be called first thing in EmitFixedPointConversion, and if it > returns non-null it uses that value instead. It's a bit unfortunate in this > instance as the only thing that doesn't work for us is the saturation, but > letting you configure *parts* of the emission is a bit too specific. > > In https://reviews.llvm.org/D50616#1203635, @rjmccall wrote: > > > If this is more than just a hobby — like if there's a backend that wants to > > do serious work with matching fixed-point operations to hardware support — > > we should just add real LLVM IR support for fixed-point types instead of > > adding a bunch of frontend customization hooks. I don't know what better > > opportunity we're expecting might come along to justify that, and I don't > > think it's some incredibly onerous task to add a new leaf to the LLVM type > > hierarchy. Honestly, LLVM programmers across the board have become far too > > accustomed to pushing things opaquely through an uncooperative IR with an > > obscure muddle of intrinsics. > > > > In the meantime, we can continue working on this code. Even if there's > > eventually real IR support for fixed-point types, this code will still be > > useful; it'll just become the core of some legalization pass in the generic > > backend. > > > It definitely is; our downstream target requires it. As far as I know there's > no real use case upstream, which is why it's likely quite hard to add any > extensions to LLVM proper. Our implementation is done through intrinsics > rather than an extension of the type system, as fixed-point numbers are > really just integers under the hood. We've always wanted to upstream our > fixed-point support (or some reasonable adaptation of it) but never gotten > the impression that it would be possible since there's no upstream user. > > A mail I sent to the mailing list a while back has more details on our > implementation, including what intrinsics we've added: > http://lists.llvm.org/pipermail/cfe-dev/2018-May/058019.html We also have an > LLVM pass that converts these intrinsics into pure IR, but it's likely that > it won't work properly with some parts of this upstream design. > > Leonard's original proposal for this work has always been Clang-centric and > never planned for implementing anything on the LLVM side, so we need to make > due with what we get, but we would prefer as small a diff from upstream as > possible. Has anyone actually asked LLVM whether they would accept fixed-point types into IR? I'm just a frontend guy, but it seems to me that there are advantages to directly representing these operations in a portable way even if there are no in-tree targets providing special support for them. And there are certainly in-tree targets that could provide such support if someone was motivated to do it. Repository: rC Clang https://reviews.llvm.org/D50616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression
rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCMac.cpp:6788 + "emitting protocol metadata without definition"); + PD = PD->getDefinition(); What happens in the `@implementation` case (the one that we're not diagnosing yet) when the protocol is a forward declaration? Repository: rC Clang https://reviews.llvm.org/D49462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/CodeGen/CGObjCMac.cpp:6788 + "emitting protocol metadata without definition"); + PD = PD->getDefinition(); arphaman wrote: > rjmccall wrote: > > What happens in the `@implementation` case (the one that we're not > > diagnosing yet) when the protocol is a forward declaration? > We emit an `external global` reference to the protocol metadata using > `GetOrEmitProtocolRef`, so this assertion won't be triggered until we force > the emission of protocol metadata from implementation as planned in a > follow-up patch. Okay. I mean, that's also unfortunate behavior, since protocol descriptors are basically `linkonce` and should be emitted in every translation unit that uses them, but I agree it's less damaging than the behavior for `@protocol`, and it means this assertion is safe. Repository: rC Clang https://reviews.llvm.org/D49462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast
rjmccall added a comment. >> Has anyone actually asked LLVM whether they would accept fixed-point types >> into IR? I'm just a frontend guy, but it seems to me that there are >> advantages to directly representing these operations in a portable way even >> if there are no in-tree targets providing special support for them. And >> there are certainly in-tree targets that could provide such support if >> someone was motivated to do it. > > I haven't brought this up to llvm-dev, but the main reason for why we decided > to only implement this in clang was because we thought it would be a lot > harder pushing a new type into upstream llvm, especially since a lot of the > features can be supported on the frontend using clang's existing > infrastructure. We are open to adding fixed point types to LLVM IR in the > future though once all the frontend stuff is laid out and if the community is > willing to accept it. Mostly I'm reluctant to add a ton of customization points to get more optimizable IR patterns from the frontend when I think it's really clear that the right thing to do is to represent these operations semantically through LLVM IR. If LLVM people don't want to add an `llvm::FixedPointType` then we should at least add portable intrinsics for them instead of target intrinsics, in which case there's still no point in customizing this in the frontend. Repository: rC Clang https://reviews.llvm.org/D50616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast
rjmccall added a comment. In https://reviews.llvm.org/D50616#1203772, @lebedev.ri wrote: > In https://reviews.llvm.org/D50616#1203751, @rjmccall wrote: > > > In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote: > > > > > > > > > > > Has anyone actually asked LLVM whether they would accept fixed-point types > > into IR? I'm just a frontend guy, but it seems to me that there are > > advantages to directly representing these operations in a portable way even > > if there are no in-tree targets providing special support for them. And > > there are certainly in-tree targets that could provide such support if > > someone was motivated to do it. > > > Even just adding one new LLVM IR instruction (well, intrinsic too, ideally) > already 'requires' you to to > then go around and make sure it is properly handled wrt all the other > instructions, optimizations, codegen. > > Adding a whole new type, i suspect, would be *much* more impactful. > And since it can already be represented via existing operations on existing > integer type, > it isn't obvious why that would be the right way forward. Very few things in LLVM actually try to exhaustively handle all operations. There are a couple of generic predicates on `Instruction` like `mayHaveSideEffects`, there's serialization/`.ll`-writing, and there's code-gen. The first two are not hard at all to implement, and it'd be quite simple to write a legalization pass in code-gen that turns all these operations into integer operations and which could easily be customized to support targets that want to do something more precise. The advantages of having real IR support include: - It's significant simpler for frontends. All of this logic in Clang will need to be reimplemented in any other frontend that wants to support fixed-point types. - It's much easier to test. The frontend needs to handle a lot of different code patterns that ultimately turn into the same small set of basic operations, like compound arithmetic and atomic operations and a million different things that are supposed to trigger implicit conversions. It's ver frustrating to write tests for these things when constants aren't readable and the generated IR for every operation is a ten-instruction sequence. - It's much easier to analyze and optimize. I'm sure some fixed-point optimizations can be done generically over the underlying integer ops, but many others would be much more difficult if not impossible — e.g. I would assume that the lowering of padded unsigned values exploits an assumption that the padding bit is zero, which a generic optimization cannot know. - All those ten-instruction sequences add up in their compile-time impact on every stage of the pipeline. I'm not saying it's an open-and-shut case, but LLVM is supposed to be an abstraction layer over more than just the concerns of code-generation, and even those concerns don't obviously point towards frontend expansion. Like I said, if this is just a hobby and we don't really care about supporting this as a feature beyond just checking a box, frontend expansion is definitely the easier approach for checking that box. But if we want a high-quality implementation of fixed-point types, we should represent them directly in LLVM IR. Repository: rC Clang https://reviews.llvm.org/D50616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast
rjmccall added a comment. In https://reviews.llvm.org/D50616#1206181, @leonardchan wrote: > I made a post on llvm-dev > (http://lists.llvm.org/pipermail/llvm-dev/2018-August/125433.html) to see if > other people have opinions on this. In the meantime, do you think I should > make a separate patch that moves this into an LLVM intrinsic function? Yeah, I think that even if LLVM decides they don't want to include first-class IR support for fixed-point types, it makes more sense to provide standard intrinsics for these operations than to do all of the lowering in the frontend. Repository: rC Clang https://reviews.llvm.org/D50616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50527: [Parser] Support alternative operator token keyword args in Objective-C++
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D50527#1206460, @erik.pilkington wrote: > Ping! If the build came back clean, then I think our combination of previous sign-offs is good enough. :) https://reviews.llvm.org/D50527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51025: [CodeGen] Fix handling of variables captured by a block that is nested inside a lambda
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D51025 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. In https://reviews.llvm.org/D44539#1208780, @QF5690 wrote: > In https://reviews.llvm.org/D44539#1207982, @rjmccall wrote: > > > LGTM. > > > Thanks! What I should do next? Haven't found any info in docs about it :) https://llvm.org/docs/Contributing.html It's up to you. We can certainly commit it for you, but if you're likely to work on more patches, you can ask for commit privileges yourself. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. Please read the developer policy: https://llvm.org/docs/DeveloperPolicy.html The information is on that page. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46475: [HIP] Set proper triple and offload kind for the toolchain
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. https://reviews.llvm.org/D46475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1998 +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + Thank you for adding this documentation. Please do clarify what the memory ordering semantics actually are when the atomic object does not need to be updated, though, and verify that target code generation actually obeys that ordering. For example, if the memory ordering makes this a release operation, `__atomic_fetch_min` must always store the result back to the atomic object, even if the new value was actually greater than the stored value; I believe that would not be required with a relaxed operation. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers
rjmccall added a comment. I agree that the format-specifier checker is not intended to be a portability checker. Any attempt to check portability problems has to account for two things: - Not all code is intended to be portable. If you're going to warn about portability problems, you need some way to not annoy programmers who might have good reason to say that they only care about their code running on Linux / macOS / 64-bit / 32-bit / whatever. Generally this means splitting the portability warning out as a separate warning group. - There are always established idioms for solving portability problems. In this case, a certain set of important typedefs from the C standard have been blessed with dedicated format modifiers like "z", but every other typedef in the world has to find some other solution, either by asserting that some existing format is good enough (as NSInteger does) or by introducing a macro that expands to an appropriate format (as some things in POSIX do). A proper format-portability warning would have to know about all of these conventions (in order to check that e.g. part of the format string came from the right macro), which just seems wildly out-of-scope for a compiler warning. Repository: rC Clang https://reviews.llvm.org/D42933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers
rjmccall added a comment. In https://reviews.llvm.org/D42933#1092077, @smeenai wrote: > Apple's current recommendation for using printf with the NSInteger types is > casting to a long, per > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html. > Are you suggesting that recommendation would change to using `%zd` instead? I'm not saying that the recommendation would change, but `long` being pointer-sized is a pretty universal assumption on Darwin, I'm afraid. In https://reviews.llvm.org/D42933#1092154, @jfb wrote: > I don't disagree with the original intent, but AFAICT that's exactly the > intent of the warning I'd like to get rid of. I'm making a very narrow point: > there is no portability problem with the warning I'm specifically trying to > silence (using `%z` with `NS[U]Integer` on Darwin). If we decide that > `-Wformat` shouldn't emit portability warnings then my point is moot, so > let's see if people agree to that. If not, then my point still stands. Agreed. >>> - There are always established idioms for solving portability problems. In >>> this case, a certain set of important typedefs from the C standard have >>> been blessed with dedicated format modifiers like "z", but every other >>> typedef in the world has to find some other solution, either by asserting >>> that some existing format is good enough (as NSInteger does) or by >>> introducing a macro that expands to an appropriate format (as some things >>> in POSIX do). A proper format-portability warning would have to know about >>> all of these conventions (in order to check that e.g. part of the format >>> string came from the right macro), which just seems wildly out-of-scope for >>> a compiler warning. > > We could provide a macro for `NS[U]Integer`, but it's been long enough and > there's enough existing code with `%z`. Not sure the code churn on user is > worth it, if we can instead say "`%z` works". Right. I'm not trying to argue that we should add a macro for NSInteger, I'm saying that that's the sort of thing that a portability warning would have to consider. Portability warnings are... always tempting, but they're very, very difficult in C. Since we're not going to do that sort of work, we should back off from doing a portability warning. John. Repository: rC Clang https://reviews.llvm.org/D42933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46613: _Atomic of empty struct shouldn't assert
rjmccall added inline comments.
Comment at: lib/AST/ASTContext.cpp:1965
+ Width = Target->getCharWidth();
+ Align = Target->getCharWidth();
+} else if (Width <= Target->getMaxAtomicPromoteWidth()) {
Alignment, unlike size, is definitely never 0. I think you should leave the
original alignment in place; it's a weird case, but we honor over-aligned empty
types in a bunch of other places.
Repository:
rC Clang
https://reviews.llvm.org/D46613
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46613: _Atomic of empty struct shouldn't assert
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D46613 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45900: CodeGen: Fix invalid bitcast for lifetime.start/end
rjmccall added a comment. In https://reviews.llvm.org/D45900#1093154, @yaxunl wrote: > In https://reviews.llvm.org/D45900#1083377, @rjmccall wrote: > > > Oh, I see, it's not that the lifetime intrinsics don't handle pointers in > > the alloca address space, it's that we might have already promoted them > > into `DefaultAS`. > > > > Do the LLVM uses of lifetime intrinsics actually look through these address > > space casts? I'm wondering if we might need to change how we emit the > > intrinsics so that they're emitted directly on (bitcasts of) the underlying > > allocas. > > > Some passes do not look through address space casts. Although there is > InferAddressSpace pass which can eliminate the redundant address space casts, > still it is desirable not to emit redundant address space in Clang. > > To avoid increasing complexity of alloca emitting API, I think we need a way > to track the original alloca and the alloca casted to default address space. > I can think of two ways: > > 1. add OriginalPointer member to Address, which is the originally emitted > LLVM value for the variable. Whenever we pass the address of a variable we > also pass the original LLVM value. > 2. add a map to CodeGenFunction to map the casted alloca to the real alloca. > > Any suggestion? Thanks. Can we just call CreateLifetimeStart (and push the cleanup to call CreateLifetimeEnd) immediately after creating the alloca instead of waiting until later like we do now? Modifying Address is not appropriate, and adding a map to CGF would be big waste. https://reviews.llvm.org/D45900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46643: CodeGen: Emit string literal in constant address space
rjmccall added a comment. The part about string literals looks fine, but: Comment at: lib/CodeGen/CGDecl.cpp:1375 +Loc = Address(EmitCastToVoidPtrInAllocaAddrSpace(Loc.getPointer()), + Loc.getAlignment()); I don't understand why a patch about string literals is changing auto variable emission. https://reviews.llvm.org/D46643 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46651: [OpenCL] Support placement new/delete in Sema
rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:2030 + } +} I think a better interpretation of this rule would be to just error on attempts to use the standard non-placement operator new/delete instead of trying to outlaw the operator declarations. For example, I don't know why a user-defined non-global operator new would be problematic. Repository: rC Clang https://reviews.llvm.org/D46651 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization
rjmccall added inline comments.
Comment at: lib/CodeGen/CGExprConstant.cpp:1413
+ } else if (!Init->isEvaluatable(CE.CGM.getContext())) {
+return false;
+ } else if (InitTy->hasPointerRepresentation()) {
sepavloff wrote:
> rsmith wrote:
> > sepavloff wrote:
> > > rjmccall wrote:
> > > > Aren't the null-pointer and integer-constant-expression checks below
> > > > already checking this? Also, `isEvaluatable` actually computes the
> > > > full value internally (as an `APValue`), so if you're worried about the
> > > > memory and compile-time effects of producing such a value, you really
> > > > shouldn't call it.
> > > >
> > > > You could reasonably move this entire function to be a method on `Expr`
> > > > that takes an `ASTContext`.
> > > Comment for `EvaluateAsRValue` says that it tries calculate expression
> > > agressively. Indeed, for the code:
> > > ```
> > > decltype(nullptr) null();
> > > int *p = null();
> > > ```
> > > compiler ignores potential side effect of `null()` and removes the call,
> > > leaving only zero initialization. `isNullPointerConstant` behaves
> > > similarly.
> > Nonetheless, it looks like this function could evaluate `Init` up to three
> > times, which seems unreasonable. Instead of the checks based on trying to
> > evaluate the initializer (`isNullPointerConstant` +
> > `isIntegerConstantExpr`), how about calling `VarDecl::evaluateValue()`
> > (which will return a potentially pre-computed and cached initializer value)
> > and checking if the result is a zero constant?
> >
> > In fact, `tryEmitPrivateForVarInit` already does most of that for you, and
> > the right place to make this change is probably in
> > `tryEmitPrivateForMemory`, where you can test to see if the `APValue` is
> > zero-initialized and produce a `zeroinitializer` if so. As a side-benefit,
> > putting the change there will mean we'll also start using `zeroinitializer`
> > for zero-initialized subobjects of objects that have non-zero pieces.
> An important point in this patch is that CodeGen tries to find out, if the
> initializer can be replaced with zeroinitializer, *prior* to the evaluation
> of it. For huge arrays the evaluation consumes huge amount of memory and time
> and it must be avoided.
>
> With this patch CodeGen may evaluate parts of the initializer, if it is
> represented by `InitListExpr`. It may cause redundant calculation, for
> instance if the check for zero initialization failed but the initializer is
> constant. To avoid this redundancy we could cache result of evaluation in
> instances of `Expr` and then use the partial values in the evaluation of the
> initializer. The simple use case solved by this patch probably is not a
> sufficient justification for such redesign.
I really think you should have some sort of type restriction in front of this
so that you don't end up creating a huge APValue only to throw it away because
it's not an int or a pointer. It's quite possible to have something like an
array initializer in a nested position that's not within an InitListExpr , e.g.
due to compound literals or std::initializer_list.
Repository:
rC Clang
https://reviews.llvm.org/D46241
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46665: [Itanium] Emit type info names with external linkage.
rjmccall added inline comments.
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:3098
+InfoLinkage = getTypeInfoLinkage(CGM, Ty);
+NameLinkage = getTypeInfoLinkage(CGM, Ty, /*ForName*/ true);
+ }
I think we could probably just have the function return both instead of
requiring the callee to make two calls.
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:3110
ItaniumCXXABI::RTTIUniquenessKind RTTIUniqueness =
- CXXABI.classifyRTTIUniqueness(Ty, Linkage);
+ CXXABI.classifyRTTIUniqueness(Ty, InfoLinkage);
if (RTTIUniqueness != ItaniumCXXABI::RUK_Unique) {
This should be the name linkage, I think. The targets using non-unique RTTI
names (i.e. Darwin ARM64) would want this bit to be set for the types they use
non-unique RTTI names for, i.e. for external types with default visibility,
even if the type is incomplete in the current translation unit and thus the
type_info object has been demoted to internal linkage. (A complete type with
internal linkage should not have the bit set.)
https://reviews.llvm.org/D46665
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46665: [Itanium] Emit type info names with external linkage.
rjmccall added inline comments. Comment at: lib/CodeGen/ItaniumCXXABI.cpp:3098 +InfoLinkage = getTypeInfoLinkage(CGM, Ty); +NameLinkage = getTypeInfoLinkage(CGM, Ty, /*ForName*/ true); + } rjmccall wrote: > I think we could probably just have the function return both instead of > requiring the callee to make two calls. "caller" instead of "callee", of course. https://reviews.llvm.org/D46665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46665: [Itanium] Emit type info names with external linkage.
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks really good, thanks. https://reviews.llvm.org/D46665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1998 +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + rjmccall wrote: > Thank you for adding this documentation. Please do clarify what the memory > ordering semantics actually are when the atomic object does not need to be > updated, though, and verify that target code generation actually obeys that > ordering. For example, if the memory ordering makes this a release > operation, `__atomic_fetch_min` must always store the result back to the > atomic object, even if the new value was actually greater than the stored > value; I believe that would not be required with a relaxed operation. Okay, that's not what I was asking for. It's okay to assume that people understand the basic memory orderings; you don't need to copy/paste generic descriptions of them here. There is something special about min/max vs. the rest of these atomic update operations, however, which is that min and max don't always change the value of the variable. (Technically this is true of corner cases of many of the other operations — for example, you could atomically add 0 to a variable or multiply a variable by 1 — but the basic operation of min/max makes this corner case a lot more important.) I am just asking you to state definitively whether the atomic operation is still appropriately ordered if the object's value does not change. If you don't understand what I'm getting at here, maybe you should just add the relaxed ordering for now. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Repository: rC Clang https://reviews.llvm.org/D46241 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46643: CodeGen: Emit string literal in constant address space
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1375 +Loc = Address(EmitCastToVoidPtrInAllocaAddrSpace(Loc.getPointer()), + Loc.getAlignment()); yaxunl wrote: > rjmccall wrote: > > I don't understand why a patch about string literals is changing auto > > variable emission. > It is a bug about alloca revealed by the lit test > > > ``` > char l_array[] = "l_array"; > > ``` > Loc contains the alloca casted to default address space, therefore it needs > to be casted back to alloca address space here, otherwise CreateBitCast > returns invalid bitcast. Unlike lifetime.start, memcpy does not require > alloca address space, so an alternative fix is to let BP take address space > of Loc. Yeah, I think using the address space of Loc is more appropriate. https://reviews.llvm.org/D46643 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0
rjmccall added a comment. Can we suppress this optimization only when we can't emit an alias? An alias shouldn't degrade debugging experience, and it's good to emit less code at -O0. Repository: rC Clang https://reviews.llvm.org/D46685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks. Repository: rC Clang https://reviews.llvm.org/D46685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages.
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.
This seems like a good idea to me. I wonder if it would be better to take
advantage of this to shrink the string tables by preserving the substitution
structure until runtime, but that really doesn't need to be part of this first
try.
Comment at: utils/TableGen/ClangDiagnosticsEmitter.cpp:514
+ std::vector Diags = Records.getAllDerivedDefinitions("Diagnostic");
+ llvm::for_each(Diags, [&](Record *R) { substituteDiagText(R, SubsMap); });
I see why this has to be done separately, I think, but it should at least go in
a helper function.
Also, please check for substitution-name conflicts.
https://reviews.llvm.org/D46740
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that works for me. The actual semantic parts of the diff seem to have disappeared from the patch posted to Phabricator, for what it's worth. Repository: rC Clang https://reviews.llvm.org/D46386 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46643: CodeGen: Emit string literal in constant address space
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1373 + llvm::Type *BP = llvm::Type::getInt8Ty(CGM.getLLVMContext()) + ->getPointerTo(Loc.getAddressSpace()); if (Loc.getType() != BP) `CGM.Int8Ty` exists. Comment at: lib/CodeGen/CodeGenModule.cpp:3077 + return Cast; +} + This should really just be a `static` function in this file. https://reviews.llvm.org/D46643 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46643: CodeGen: Emit string literal in constant address space
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. https://reviews.llvm.org/D46643 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45900: CodeGen: Fix invalid bitcast for lifetime.start/end
rjmccall added a comment. That would work. I think it would be reasonable for AutoVarEmission to store that pointer if it makes things easier. https://reviews.llvm.org/D45900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46489: [HIP] Let assembler output bitcode for amdgcn
rjmccall added a comment. In https://reviews.llvm.org/D46489#1099979, @yaxunl wrote: > In https://reviews.llvm.org/D46489#1088940, @rjmccall wrote: > > > I think the right solution here is to make a CompileJobAction with type > > TY_LLVM_BC in the first place. You should get the advice of a driver > > expert, though. > > > There is already JobAction for TY_LLVM_BC. I just want to skip the backend > and assemble phase when offloading HIP. I will try achieving that through HIP > action builder. Right, that's what I mean. This is something we already support for LTO and other purposes. You can just check what happens in the driver if you pass `-c -emit-llvm`. https://reviews.llvm.org/D46489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46665: [Itanium] Emit type info names with external linkage.
rjmccall added a comment. I believe static and dynamic linkers — at least on ELF and Mach-O — will always drop weak symbols for strong ones. Now, I think that isn't LLVM's posted semantics for linkonce_odr, but to me that means that LLVM's semantics are inadequate, not that we should decline to take advantage of them. If we can't rely on that, it probably means that the type name symbol for class types always has to be linkonce_odr, even if it's for a type with a key function. Repository: rC Clang https://reviews.llvm.org/D46665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46665: [Itanium] Emit type info names with external linkage.
rjmccall added a comment. In https://reviews.llvm.org/D46665#1102348, @rsmith wrote: > In https://reviews.llvm.org/D46665#1102290, @rjmccall wrote: > > > I believe static and dynamic linkers — at least on ELF and Mach-O — will > > always drop weak symbols for strong ones. Now, I think that isn't LLVM's > > posted semantics for linkonce_odr, but to me that means that LLVM's > > semantics are inadequate, not that we should decline to take advantage of > > them. > > > > If we can't rely on that, it probably means that the type name symbol for > > class types always has to be linkonce_odr, even if it's for a type with a > > key function. > > > That all makes sense to me. (But I think `weak_odr` would be more formally > correct, even though the symbol will never actually be discarded as it's > referenced from the type_info and vtable.) Yes, of course. > The rule that ASan is using is that if there is an `external` definition for > a global variable, there should not be any other definitions for that symbol, > which seems right given LLVM's semantics, but wrong here. I agree. > For now, the simplest thing to do seem to be to weaken `external` linkage to > `weak_odr` for the type info name. Yes. Repository: rC Clang https://reviews.llvm.org/D46665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45900: CodeGen: Fix invalid bitcast for lifetime.start/end
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D45900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45898: [SemaCXX] Mark destructor as referenced
rjmccall added a comment. I agree that the new-expression case doesn't use the destructor, and all the other cases of list-initialization presumably use the destructor for the initialized type for separate reasons. Ok. Comment at: test/CodeGenObjCXX/arc-list-init-destruct.mm:1 +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.13.0 -std=c++1z -fobjc-arc -fobjc-exceptions -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s + ahatanak wrote: > rjmccall wrote: > > Does the corresponding C++ test case (replacing `Class0 *f;` with > > `HasExplicitNonTrivialDestructor f;`) not reproduce the problem? > I wasn't able to reproduce the problem by changing the type of field 'f' to a > C++ class with a non-trivial destructor because, if I make that change, > Class1's destructor declaration gets added in > Sema::AddImplicitlyDeclaredMembersToClass. I don't fully understand the > reason behind it, but Class1's destructor declaration is added when the type > of one of its subobject has a user-declared destructor. Interesting, alright. Repository: rC Clang https://reviews.llvm.org/D45898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0
rjmccall added a comment. Well, internal and external types are important cases. I'm fine with this. It's a pity that we can't express what we want with aliases, though. Repository: rC Clang https://reviews.llvm.org/D46685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. It's waiting on a discussion that's happening pretty slowly, sorry. I know this is frustrating. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. This isn't really an Objective-C user forum, but I'll try to summarize briefly. `unsafe_unretained` is an unsafe version of `weak` — it's unsafe because it can be left dangling if the object is deallocated. It's necessary for a small (and getting smaller every year) set of classes that don't support true weak references, and it can be useful as an optimization to avoid the performance overhead of reference counting. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47092: downgrade strong type info names to weak_odr linkage
rjmccall added a comment.
Incomplete classes are a curse. I don't suppose we can just modify the
language specification to make it illegal to use `typeid(Incomplete*)`? Or
make equality/hashing undefined in these cases?
Comment at: test/CodeGenCXX/arm64.cpp:48
void A::foo() {}
- // CHECK-GLOBALS-DAG: @_ZTSN5test21AE = constant [11 x i8]
+ // CHECK-GLOBALS-DAG: @_ZTSN5test21AE = weak_odr constant [11 x i8]
// CHECK-GLOBALS-DAG: @_ZTIN5test21AE = constant { {{.*}}, i8* getelementptr
inbounds ([11 x i8], [11 x i8]* @_ZTSN5test21AE, i32 0, i32 0) }
The way the Darwin ARM64 ABI handles non-unique RTTI is by setting a flag
indicating that clients should do string comparisons/hashes; it does not rely
on the type name symbols being uniqued. So this should stay external and the
_ZTI definition should change to set the bit.
Comment at: test/CodeGenCXX/windows-itanium-type-info.cpp:31
// CHECK-DAG: @_ZTI7derived = dso_local dllexport constant
-// CHECK-DAG: @_ZTS7derived = dso_local dllexport constant
+// CHECK-DAG: @_ZTS7derived = weak_odr dso_local dllexport constant
// CHECK-DAG: @_ZTV7derived = dso_local dllexport unnamed_addr constant
Is this actually okay? I didn't think dllexport supported weak symbols.
Repository:
rC Clang
https://reviews.llvm.org/D47092
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages.
rjmccall added inline comments.
Comment at: utils/TableGen/ClangDiagnosticsEmitter.cpp:514
+ std::vector Diags = Records.getAllDerivedDefinitions("Diagnostic");
+ llvm::for_each(Diags, [&](Record *R) { substituteDiagText(R, SubsMap); });
EricWF wrote:
> rjmccall wrote:
> > I see why this has to be done separately, I think, but it should at least
> > go in a helper function.
> >
> > Also, please check for substitution-name conflicts.
> @rjmccall By substitution name conflicts do you mean substitution names which
> conflict with diagnostic names?
I meant between substitutions, but I guess that's probably handled
automatically by the base TableGen parser, sorry.
https://reviews.llvm.org/D46740
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47092: downgrade strong type info names to weak_odr linkage
rjmccall added a comment. In https://reviews.llvm.org/D47092#1105317, @rjmccall wrote: > Incomplete classes are a curse. I don't suppose we can just modify the > language specification to make it illegal to use `typeid(Incomplete*)`? Or > make equality/hashing undefined in these cases? Honestly, I'm somewhat inclined to just declare this for Darwin and make us non-conformant. It's not like this has ever previously worked, and this patch is going to be disruptive for our internal C++ developers by introducing new global weak symbols that they can't get rid of except by disabling RTTI. (It would always have been a problem if they were actually using RTTI, of course, but one of the whole problems with RTTI is that you live with the consequences whether you use it or not.) Repository: rC Clang https://reviews.llvm.org/D47092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
rjmccall added a comment. RecursiveASTVisitor instantiations are huge. Can you just make the function take a Stmt and then do the first few checks if it happens to be an Expr? Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47092: downgrade strong type info names to weak_odr linkage
rjmccall added inline comments.
Comment at: test/CodeGenCXX/arm64.cpp:48
void A::foo() {}
- // CHECK-GLOBALS-DAG: @_ZTSN5test21AE = constant [11 x i8]
+ // CHECK-GLOBALS-DAG: @_ZTSN5test21AE = weak_odr constant [11 x i8]
// CHECK-GLOBALS-DAG: @_ZTIN5test21AE = constant { {{.*}}, i8* getelementptr
inbounds ([11 x i8], [11 x i8]* @_ZTSN5test21AE, i32 0, i32 0) }
rsmith wrote:
> rjmccall wrote:
> > The way the Darwin ARM64 ABI handles non-unique RTTI is by setting a flag
> > indicating that clients should do string comparisons/hashes; it does not
> > rely on the type name symbols being uniqued. So this should stay external
> > and the _ZTI definition should change to set the bit.
> OK. I was trying to avoid introducing more cases where we do string
> comparisons, but if you prefer string comparisons over link-time
> deduplication in all cases for that target, that seems easy enough to support.
>
> (Out of curiosity, does the link-time deduplication not reliably work on that
> platform? If so, how do you deal with the other cases that require
> deduplication? If not, what's the motivation for using string comparisons?)
Static and dynamic linking work the same on ARM64 as on any other Mach-O
platform. As you say, there are language features that depend on symbols
resolving to the same object; we haven't changed that. The issue is that,
while deduplication in the *static* linker is great, deduplication in the
*dynamic* linker means extra, unavoidable work at load time — mostly, paging in
a bunch of data from the symbol table. For an implicitly-activated language
feature like RTTI, that can add up to a lot of overhead, which hits you
regardless of whether you actually ever use any of the language features that
depend on it. On Darwin we really like dynamic libraries, and we really like
processes to launch quickly. So the ARM64 ABI says that RTTI which would
otherwise need to be potentially deduplicated across a dynamic-library boundary
(because the type is external and not hidden) is instead hidden and must be
compared using the string data — a classic load-time vs. execution-time
trade-off, in this case easy to make because the features that depend on
dynamic type matching are rarely used and generally slow anyway.
The GCC type_info ABI uses string comparisons for a completely different
purpose: they assume that users can't be trusted to mark visibility correctly
on types, so they use string comparisons as a fallback. Darwin ARM64 is still
as strict about type visibility as we are on every other platform, so if you
use -fvisibility=hidden and forget to mark a type as visible, dynamic_cast and
EH will fail across library boundaries.
It occurs to me that all this might be unnecessary, though:
1. It's never possible to ask for the `type_info` of an incomplete class type
directly.
2. The features that use RTTI which can actually do pointer conversions don't
allow pointers to incomplete class types, either — at best, you can have
pointers-to-pointers.
3. As far as I know, none of those features would ever depend on the pointee
type_info object of a pointer-to-pointer. For example, you cannot do a
`dynamic_cast` from a `Base**` to a `Derived**`. They only care about the
direct pointee type, which cannot be incomplete.
4. I think the only feature that allows a pointer to an incomplete class type
is `typeid`.
5. None of the standard operations on a `type_info` actually cares about the
pointee `type_info`. They care about the name symbol for the pointer type, but
that already has to be deduplicated (modulo the ARM64 trick) because we don't
eagerly emit RTTI for pointer types.
6. You can get at the pointee `type_info` with the `cxxabi.h` interface, but I
would argue that we should not make the ABI worse solely for the benefit of
clients of the `cxxabi.h` interface, especially in a corner case of a corner
case (a pointer to an incomplete type which would have turned out to be a
dynamic class with a key function).
7. So I think we could actually get away with always using internal linkage for
the ZTSes of incomplete class types, as long as we continue to use the standard
rule for the ZTSes of *pointers* to incomplete class types, because the only
thing that would break would be the `cxxabi.h` interface.
8. And if we find a way to convince LLVM / ASan to let us use weak linkage even
if there might also be a strong definition out there, we don't even have that
problem long-term.
Repository:
rC Clang
https://reviews.llvm.org/D47092
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
rjmccall added a comment. In https://reviews.llvm.org/D47096#1105374, @jfb wrote: > In https://reviews.llvm.org/D47096#1105368, @rjmccall wrote: > > > RecursiveASTVisitor instantiations are huge. Can you just make the > > function take a Stmt and then do the first few checks if it happens to be > > an Expr? > > > I'm not super-familiar with the code, so I might be doing something silly. > > I did something like this initially (leave the top of the function as-is, and > instead of cast do dyn_cast to Expr and if that fails to CompoundStmt, > recursively iterating all the children of the CompoundStmt). My worry was > that I wasn't traversing all actual children (just CompountStmt's children), > and AFAICT there's no easy way to say "take any Stmt, and visit its children > if it has such a method". I could hard-code more Stmt derivatives but that > seems brittle, I could use the "detection idiom" but that's silly if there's > already a visitor which does The Right Thing through tablegen magic. > > What I can do is what I did earlier, and conservatively say it was captured > if it's neither an Expr nor a CompoundStmt? Or should I special-case other > things as well? `children()` is actually defined at the `Stmt` level, and if you look at how it's implemented on e.g. `IfStmt`, you can see that it visits all of the child `Stmt`s, including the if-condition. So it should be fine. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
rjmccall added a comment. Test case should go in test/CodeGenCXX. Also, there already is a `blocks.cpp` there. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47099: Disable casting of alloca for ActiveFlag
rjmccall added a comment. Maybe there should just be a method that makes a primitive alloca without the casting, and then you can call that in CreateTempAlloca. https://reviews.llvm.org/D47099 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47099: Disable casting of alloca for ActiveFlag
rjmccall added a comment. In https://reviews.llvm.org/D47099#1105574, @yaxunl wrote: > In https://reviews.llvm.org/D47099#1105493, @rjmccall wrote: > > > Maybe there should just be a method that makes a primitive alloca without > > the casting, and then you can call that in CreateTempAlloca. > > > In many cases we still need to call CreateTempAlloca with cast enabled, since > we are not certain there is only load from it and store to it. Any time it is > stored to another memory location or passed to another function (e.g. > constructor/destructor), it needs to be a pointer to the language's default > address space since the language sees it that way. Yes, I understand that. But there are some cases, like this, that do not need to produce something in `LangAS::Default`, and extracting out a method that just does an `alloca` without the unnecessary cast seems sensible. > An alternative fix would be just let ActiveFlag to be llvm::Value instead of > llvm::AllocaInst, since it does not really need to be an AllocaInst. I don't care at all about the type of `ActiveFlag`, but if we can generate better code at -O0 by avoiding the casts in situations where we obviously don't need them, we should do it. https://reviews.llvm.org/D47099 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46052: GNUstep Objective-C ABI version 2
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, my comments seem to all be addressed. Repository: rC Clang https://reviews.llvm.org/D46052 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. This was approved by the Objective-C language group as a default-off warning. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1018 +def warn_objc_property_assign_on_object : Warning< + "'assign' attribute must not be of object type, use 'unsafe_unretained' instead">, + InGroup, DefaultIgnore; "must" is rather strong for a warning. Maybe something more like "'assign' attribute on property of object type could be 'unsafe_unretained'"? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. In https://reviews.llvm.org/D44539#1106443, @aaron.ballman wrote: > In https://reviews.llvm.org/D44539#1106152, @rjmccall wrote: > > > This was approved by the Objective-C language group as a default-off > > warning. > > > We usually do not expose new default-off warnings because experience shows > that they rarely ever get enabled by users. If the ObjC group doesn't think > this should be on by default, I wonder if it should be included in Clang at > all. That's a fair question to ask. In this case, I'm in favor of adding it because we have evidence of there being a substantial set of users who would enable it enthusiastically. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1018 +def warn_objc_property_assign_on_object : Warning< + "'assign' attribute must not be of object type, use 'unsafe_unretained' instead">, + InGroup, DefaultIgnore; QF5690 wrote: > rjmccall wrote: > > "must" is rather strong for a warning. Maybe something more like "'assign' > > attribute on property of object type could be 'unsafe_unretained'"? > But "could be" is rather weak :) > May be "Prefer using explicit 'unsafe_unretained' attribute instead of > 'assign' for object types", or "Using explicit 'unsafe_unretained' attribute > instead of 'assign' for object types is preferred" (if passive voice is > preferred) Neither of those is quite in the standard diagnostic "voice". Maybe something like "'assign' property of object type may become a dangling reference; consider using 'unsafe_unretained'"? Oh, you should probably not warn about `Class` types. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47099: Disable casting of alloca for ActiveFlag
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:80 auto Alloca = CreateTempAlloca(Ty, Name, ArraySize); Alloca->setAlignment(Align.getQuantity()); if (AllocaAddr) Could you change this to call CreateTempAllocaWithoutCast? https://reviews.llvm.org/D47099 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47099: Call CreateTempAllocaWithoutCast for ActiveFlag
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. https://reviews.llvm.org/D47099 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably
rjmccall added a comment. I like this approach a lot. Comment at: lib/CodeGen/CGExprConstant.cpp:675 + // We have mixed types. Use a packed struct. + std::vector Types; + Types.reserve(Elements.size()); Why std::vector? Repository: rC Clang https://reviews.llvm.org/D47166 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
