rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (286 w, 5 d)

Recent Activity

Yesterday

rjmccall added a comment to D49508: [Sema] Expr::skipRValueSubobjectAdjustments(): record skipped NoOp casts..

I think it would be reasonable to set a flag on ImplicitCastExprs that are actually semantically part of an explicit cast. I don't think that would be hard to get Sema to do, either by passing a flag down to the code that builds those casts or just by retroactively setting that flag on all the ICE sub-expressions of an explicit cast when "capping" it with the ExplicitCastExpr.

Wed, Jul 18, 9:11 PM
rjmccall added inline comments to D48661: [Fixed Point Arithmetic] Fixed Point Constant.
Wed, Jul 18, 9:06 PM · Restricted Project
rjmccall added a comment to D49083: [HIP] Register/unregister device fat binary only once.

Thanks for the comment.

Wed, Jul 18, 9:04 PM
rjmccall added inline comments to D49083: [HIP] Register/unregister device fat binary only once.
Wed, Jul 18, 10:49 AM
rjmccall added inline comments to D49294: Sema: Fix explicit address space cast in C++.
Wed, Jul 18, 10:48 AM

Tue, Jul 17

rjmccall added inline comments to D49457: DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array.
Tue, Jul 17, 7:51 PM
rjmccall added a comment to D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression.

Hmm. I think this is a reasonable change to make to the language. Have you investigated to see if this causes source-compatibility problems?

Tue, Jul 17, 6:58 PM
rjmccall added inline comments to D49457: DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array.
Tue, Jul 17, 6:50 PM
rjmccall added a comment to D49303: [CodeGen][ObjC] Treat non-escaping blocks as global blocks to make copy/dispose a no-op.

Please test that we still copy captures correctly into the block object and destroy them when the block object is destroyed.

Tue, Jul 17, 4:38 PM
rjmccall added a comment to D49085: [Sema] Emit a diagnostic for an invalid dependent function template specialization.

This is the right basic approach. I think it would be better if the diagnostic text was more like err_function_template_spec_no_match, maybe "no candidate function template was found for dependent friend function template specialization". And it would be good to emit notes on any declarations we found but discarded.

Tue, Jul 17, 4:10 PM
rjmccall added inline comments to D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types.
Tue, Jul 17, 3:43 PM
rjmccall added a comment to D49403: More aggressively complete RecordTypes with Function Pointers.

Can you explain why this is important for the optimizer?

Tue, Jul 17, 3:38 PM
rjmccall added inline comments to D48661: [Fixed Point Arithmetic] Fixed Point Constant.
Tue, Jul 17, 3:34 PM · Restricted Project
rjmccall added inline comments to D49294: Sema: Fix explicit address space cast in C++.
Tue, Jul 17, 3:27 PM
rjmccall added inline comments to D49083: [HIP] Register/unregister device fat binary only once.
Tue, Jul 17, 3:22 PM

Sat, Jul 7

rjmccall added inline comments to D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.
Sat, Jul 7, 11:00 PM · Restricted Project
rjmccall accepted D49042: [LangRef] Clarify alloca of zero bytes..

LGTM.

Sat, Jul 7, 10:57 PM

Tue, Jul 3

rjmccall added a comment to D48661: [Fixed Point Arithmetic] Fixed Point Constant.

Thanks, much appreciated. A couple more style changes that I noticed and this will be LGTM, although you should also make sure you have Bevin Hansson's approval.

Tue, Jul 3, 6:23 PM · Restricted Project
rjmccall added a comment to D45898: [SemaCXX] Mark destructor as referenced .

LGTM, but I'd like Richard to sign off, too.

Tue, Jul 3, 11:15 AM
rjmccall added inline comments to D48661: [Fixed Point Arithmetic] Fixed Point Constant.
Tue, Jul 3, 10:49 AM · Restricted Project

Mon, Jul 2

rjmccall added inline comments to D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers.
Mon, Jul 2, 11:50 AM

Thu, Jun 28

rjmccall added inline comments to D48661: [Fixed Point Arithmetic] Fixed Point Constant.
Thu, Jun 28, 7:51 PM · Restricted Project
rjmccall accepted D48727: [Fixed Point Arithmetic] Rename `-fsame-fbits` flag.

Oh, having the same number of fractional bits is what leads to unsigned types having one bit of padding, and vice versa.

If this flag is set to false, then the integral and fractional parts of the unsigned types take up the whole bit width of the underlying scaled integer, with unsigned types gaining one extra fractional bit which would be taken by the sign bit in the signed types. When this flag is true, the number of fractional bits for unsigned types changes to match the signed types, but the number of integral bits stays the same, which leads to one unused padding bit.

Thu, Jun 28, 9:53 AM · Restricted Project
rjmccall added a comment to D48727: [Fixed Point Arithmetic] Rename `-fsame-fbits` flag.

This all looks reasonable except that I think the interpretation is exactly backwards, no? From the documentation on the option, -fpadding-on-unsigned-fixed-point causes there to be padding, i.e. the inverse of the old SameFBits; but the default value, comments, and boolean checks throughout the code are all still using the old interpretation.

Thu, Jun 28, 9:32 AM · Restricted Project
rjmccall added inline comments to D48661: [Fixed Point Arithmetic] Fixed Point Constant.
Thu, Jun 28, 9:24 AM · Restricted Project

Wed, Jun 27

rjmccall added a comment to D48589: [WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand.

@rjmccall because we do not want to impact the clients of ABIArgInfo I thought of two possible approaches

  1. extend CGFunctionInfo with a third trailing array (now it has two), with as many elements as ABIArgInfo (call it ABIArgExtraInfo) then during the creation of CGFunctionInfo link (somehow, see discussion below) each ABIArgInfoto its corresponding ABIArgExtraInfo.
  2. add a dynamic container like a SmallVector<ABIArgExtraInfo, 2> in CGFunctionInfo and allocate ABIArgExtraInfo as needed.

    In both cases during getFOO, ABIArgInfo would put the extra (less commonly used) data into the corresponding ABIArgExtraInfo (overflowing object).

    In both cases too, we need a way to navigate to our enclosing CGFunctionInfo but strictly only during the getFOO that fills the ABIArgInfo. Getting it from the users is not possible, so we would need a pointer to CGFunctionInfo in ABIArgInfo (set up during the creation of CGFunctionInfo). That said, there is no need of its value all the time, only at the beginning of getFOO. We could reuse its storage to point the overflowing object (if needed, and set it to null otherwise).

    Do any of the approaches look any close to what you had in mind? Perhaps I'm missing the point.
Wed, Jun 27, 11:40 AM
rjmccall added inline comments to D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals.
Wed, Jun 27, 11:18 AM · Restricted Project
rjmccall added inline comments to D48661: [Fixed Point Arithmetic] Fixed Point Constant.
Wed, Jun 27, 11:05 AM · Restricted Project
rjmccall added inline comments to D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types.
Wed, Jun 27, 12:51 AM · Restricted Project

Tue, Jun 26

rjmccall added inline comments to D48589: [WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand.
Tue, Jun 26, 2:22 PM
rjmccall added inline comments to D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types.
Tue, Jun 26, 1:07 PM · Restricted Project
rjmccall added inline comments to D48589: [WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand.
Tue, Jun 26, 12:05 PM

Sun, Jun 24

rjmccall added inline comments to D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types.
Sun, Jun 24, 10:35 PM · Restricted Project
rjmccall accepted D48531: [CodeGen] Provide source locations for UBSan type checks when emitting constructor calls..

LGTM.

Sun, Jun 24, 10:18 PM · Restricted Project

Fri, Jun 22

rjmccall added inline comments to D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types.
Fri, Jun 22, 3:29 PM

Thu, Jun 21

rjmccall added inline comments to D48386: [clang] Mark libm functions writeonly when we care about errno.
Thu, Jun 21, 8:25 PM
rjmccall added a comment to D48386: [clang] Mark libm functions writeonly when we care about errno.

Basic approach seems reasonable to me.

Thu, Jun 21, 8:22 PM

Wed, Jun 20

rjmccall added a comment to rC335021: [Sema] Produce diagnostics for attribute 'trivial_abi' that appears.

Functionally seems fine to me.

Wed, Jun 20, 3:40 PM

Jun 18 2018

rjmccall added a comment to D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions.
In D47988#1135929, @rnk wrote:

In general, it's unfortunate that this has to leave so many C++-runtime-specific tendrils in the ObjC code. Unlike the EH type patch, though, I'm not sure I can see a great alternative here, especially because of the semantic restrictions required by outlining.

It's technically possible to lift those restrictions by returning an i32 from the outlined function and switching on it. Right? The question is, is it worth it? The catch funclet would effectively store the i32 to the stack frame, then "catchret" via the runtime, and then we'd switch out to the jump target.

Jun 18 2018, 5:06 PM
rjmccall added a comment to D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions.

In general, it's unfortunate that this has to leave so many C++-runtime-specific tendrils in the ObjC code. Unlike the EH type patch, though, I'm not sure I can see a great alternative here, especially because of the semantic restrictions required by outlining.

Jun 18 2018, 11:33 AM
rjmccall added inline comments to D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types.
Jun 18 2018, 11:20 AM

Jun 15 2018

rjmccall added inline comments to D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers.
Jun 15 2018, 3:41 PM

Jun 11 2018

rjmccall added inline comments to D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers.
Jun 11 2018, 11:32 PM
rjmccall added a comment to D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types.

Wrapping an Objective-C exception inside a C++ exception means dynamically constructing a C++ exception object and traversing the class hierarchy of the thrown Obj-C object to populate the catchable types array of the C++ exception. Microsoft's C++ runtime will perform the appropriate catch type comparisons, and this patch makes the compiler emit the right typeinfos into the exception handling tables for all of that to work. https://github.com/Microsoft/libobjc2/blob/f2e4c5ac4b3ac17f413a38bbc7ee1242f9efd0f7/msvc/eh_winobjc.cc#L116 is how WinObjC does this wrapping, for example.

Jun 11 2018, 6:56 PM
rjmccall added a comment to D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types.

The non-fragile Objective-C path — i.e. interoperation with C++ exceptions instead of using setjmp/longjmp in an utterly-incompatible style — is absolutely the right direction going forward.

Jun 11 2018, 3:44 PM
rjmccall added a comment to D48040: Implement constexpr __builtin_*_overflow.

Thanks, comment change looks good. LGTM.

Jun 11 2018, 2:43 PM
rjmccall accepted D47733: [CUDA][HIP] Set kernel calling convention before arrange function.

Ok.

Jun 11 2018, 11:56 AM
rjmccall added inline comments to D48040: Implement constexpr __builtin_*_overflow.
Jun 11 2018, 11:47 AM
rjmccall accepted D46651: [OpenCL] Support new/delete in Sema.

LGTM.

Jun 11 2018, 11:41 AM
rjmccall added a comment to D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces..

Well, the documentation mismatch is worth fixing even if the code isn't. But I think at best your use-case calls for weakening the assertion to be that any existing address space isn't *different*, yeah.

Alright, I'll give that a shot.

Separately, I'm not sure that's really the right representation for a Harvard architecture (which is what I assume you're trying to extend Clang to support); I think you should probably just teach the compiler that function pointers are different.

Well, we've already implemented it and it's been running in our downstream for a while without issues at this point. We just figured it was less work to use the existing address space support for it than to hack special cases all over the place for functions and function pointers.

Jun 11 2018, 11:38 AM

Jun 9 2018

rjmccall added a reviewer for D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments: t.p.northover.

Okay, as a code change this seems more reasonable to me. I haven't really thought through the ABI-compatibility issues, though. CC'ing Tim.

Jun 9 2018, 7:41 PM
rjmccall added a comment to D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces..

There's actually a bit more to it than in these two patches. The complete diff to this function in our downstream Clang looks like this:

 QualType
 ASTContext::getAddrSpaceQualType(QualType T, unsigned AddressSpace) const {
-  QualType CanT = getCanonicalType(T);
-  if (CanT.getAddressSpace() == AddressSpace)
+  if (T.getLocalQualifiers().getAddressSpace() == AddressSpace)
     return T;
 
   // If we are composing extended qualifiers together, merge together
   // into one ExtQuals node.
   QualifierCollector Quals;
   const Type *TypeNode = Quals.strip(T);
 
   // If this type already has an address space specified, it cannot get
   // another one.
-  assert(!Quals.hasAddressSpace() &&
-         "Type cannot be in multiple addr spaces!");
-  Quals.addAddressSpace(AddressSpace);
+  if (Quals.hasAddressSpace())
+    Quals.removeAddressSpace();
+  if (AddressSpace)
+    Quals.addAddressSpace(AddressSpace);
 
   return getExtQualType(TypeNode, Quals);
 }
In our downstream Clang, functions have address spaces. The desired address space is applied in getFunctionType, using getAddrSpaceQualType. Due to how FunctionTypes are built, it's possible to end up with noncanonical FunctionType that doesn't have an address space whose canonical type does have one. For example, when building the noncanonical type `void (*)(void * const)`, we will first build its canonical type `void (_AS *)(void *)`. Since getAddrSpaceQualType looks at the canonical type to determine whether the address space is already applied, it's skipped and we end up with this discrepancy.

Now that I test it again, I can't seem to induce the assertion. I suspect I might just have changed it because the documentation said that was how it was supposed to work. Perhaps I should be submitting the upper diff instead? Though, considering that this cannot be reproduced in trunk maybe I simply shouldn't submit it at all.

Jun 9 2018, 7:35 PM
rjmccall added inline comments to D46651: [OpenCL] Support new/delete in Sema.
Jun 9 2018, 7:28 PM
rjmccall added inline comments to D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.
Jun 9 2018, 7:23 PM · Restricted Project

Jun 4 2018

rjmccall accepted D47694: [CUDA][HIP] Do not emit type info when compiling for device.

Thanks, LGTM.

Jun 4 2018, 8:53 PM
rjmccall added a comment to D47694: [CUDA][HIP] Do not emit type info when compiling for device.

Oh, I see, because you're worried that the host code might contain dynamic_cast or similar features which would complain if RTTI were disabled.

Jun 4 2018, 3:37 PM
rjmccall accepted D47564: [Parse] Use CapturedStmt for @finally on MSVC.

Okay, We can try this, then.

Jun 4 2018, 3:26 PM
rjmccall added a comment to D46042: Cap vector alignment at 16 for all Darwin platforms.
In D46042#1121648, @rnk wrote:

It's the typedef alignment changes that are causing problems for us, not the MaxVectorAlign changes. That makes more sense. The new alignment attribute breaks our implementation of _mm256_loadu_ps, because the packed struct ends up with a 32-byte alignment. Here's the implementation:

static __inline __m256 __DEFAULT_FN_ATTRS
_mm256_loadu_ps(float const *__p)
{
  struct __loadu_ps {
    __m256 __v;
  } __attribute__((__packed__, __may_alias__));
  return ((struct __loadu_ps*)__p)->__v;
}

And clang's -fdump-record-layouts says:

*** Dumping AST Record Layout
         0 | struct __loadu_ps
         0 |   __m256 __v
           | [sizeof=32, align=32]

I think the problem is that __attribute__((aligned(N))) beats __attribute__((packed)) on Windows to match MSVC's behavior with __declspec(align(N)).

I think we should revert this for now. Adding the alignment attribute to all Intel vector typedefs is a bigger change than it seems.

Jun 4 2018, 3:17 PM
rjmccall added a comment to D47694: [CUDA][HIP] Do not emit type info when compiling for device.

Why not just have the driver disable RTTI in the frontend invocation?

CUDA/HIP uses single source for device and host. The host code may depend on RTTI,
e.g., an application may include some boost headers which will fail if RTTI is disabled,
therefore RTTI cannot be disabled when compiling device code.

Jun 4 2018, 8:41 AM
rjmccall added a comment to D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces..

Well, Sema should always be diagnosing conflicts.

Jun 4 2018, 8:37 AM

Jun 3 2018

rjmccall added a comment to D47694: [CUDA][HIP] Do not emit type info when compiling for device.

Why not just have the driver disable RTTI in the frontend invocation?

Jun 3 2018, 6:54 PM

Jun 1 2018

rjmccall added a comment to D47564: [Parse] Use CapturedStmt for @finally on MSVC.

No, it was just a general question. Have you gotten this to a point where it's testable?

Yup, it's been working fine in my local testing. There's one more patch that I need to put up, which actually handles doing proper codegen for @try/@catch/@finally; I'm working on cleaning that up right now.

Jun 1 2018, 11:41 PM
rjmccall closed D46042: Cap vector alignment at 16 for all Darwin platforms.

Landed as r333791.

Jun 1 2018, 3:01 PM
rjmccall committed rC333791: Cap "voluntary" vector alignment at 16 for all Darwin platforms..
Cap "voluntary" vector alignment at 16 for all Darwin platforms.
Jun 1 2018, 2:38 PM
rjmccall committed rL333791: Cap "voluntary" vector alignment at 16 for all Darwin platforms..
Cap "voluntary" vector alignment at 16 for all Darwin platforms.
Jun 1 2018, 2:38 PM
rjmccall added a comment to D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments.

I'm not sure it's appropriate to impose this as an overhead on all targets.

You mean the overhead of increasing the size of TypeInfo? That's fair.

Jun 1 2018, 1:43 PM
rjmccall added a comment to D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments.

I'm not sure it's appropriate to impose this as an overhead on all targets.

Jun 1 2018, 1:12 PM
rjmccall added a comment to D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces..

Is there a specific reason to change the implementation instead of changing the documentation?

Jun 1 2018, 1:09 PM

May 31 2018

rjmccall added a comment to D47564: [Parse] Use CapturedStmt for @finally on MSVC.

That's an interesting idea. I don't see any particular reason not to do it this way if you're willing to accept that it's never going to support the full control-flow possibilities of @finally. You will need to add JumpDiagnostics logic to prevent branches out of the block, and I don't know how this will interact with attempts to throw an exception out.

There's already some logic in CapturedStmt to prevent branches out of the block:

  • Attempting to return will produce "cannot return from Objective-C @finally statement"
  • Attempting to goto out of the block will result in "use of undeclared label", which is a bad diagnostic (and should be improved), but it does error
May 31 2018, 6:11 PM
rjmccall added a comment to D47564: [Parse] Use CapturedStmt for @finally on MSVC.

That's an interesting idea. I don't see any particular reason not to do it this way if you're willing to accept that it's never going to support the full control-flow possibilities of @finally. You will need to add JumpDiagnostics logic to prevent branches out of the block, and I don't know how this will interact with attempts to throw an exception out.

May 31 2018, 5:00 PM

May 24 2018

rjmccall accepted D47354: [CodeGen][Darwin] Set the calling-convention of a thread-local variable initialization function to fix calling-convention mismatch.

LGTM.

May 24 2018, 8:09 PM

May 23 2018

rjmccall added inline comments to D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type.
May 23 2018, 9:12 PM · Restricted Project
rjmccall added inline comments to D47103: Implement strip.invariant.group.
May 23 2018, 9:08 PM
rjmccall added a comment to D47108: [CodeGenCXX] Add -fforce-emit-vtables.

I thought we already had places in Sema that marked inline virtual methods as used, instantiated templates, etc. for devirtualization purposes when optimization was enabled. Did we rip that out?

I only recall the emitting available_externally vtables opportunistically, that is emitting it only if all the inline virtual functions are present (and they are not hidden).
(https://reviews.llvm.org/D33437)

The problem we've had over and over with devirtualization is that we have to emit a perfect v-table because LLVM lacks a lot of the key vocabulary for talking about incomplete information. For example, if something weird happens and we don't have a definition for an inline virtual method, ideally we'd just say "well, you can't devirtualize this slot", then try to fix that as incremental progress; but instead we have to get everything just right or else disable the whole optimization. Note that vague-linkage v-tables mean that we'd also need to be able to say things like "there is an object with a definition that looks like this, but its symbol is not available and you can't emit it yourself".

That is correcty, my intention was that this flag would cause all inline virtual functions to be emitted. Can you give a hint how to achieve this in the sane way?

May 23 2018, 2:15 PM

May 22 2018

rjmccall added a comment to D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type.

There are at least three good reasons to make sure this can enabled/disabled by a flag:

  • We have to anticipate that introducing new keywords will cause some compatibility problems. New language standards that introduce new keywords can be disabled using -std=<something earlier>. We shouldn't let this bypass that just because it's an out-of-band addition to the base language.
  • It seems likely to me that this feature will be target-restricted.
  • It seems plausible to me that development of this feature might continue past the Clang 7 branch date, and we shouldn't release a compiler with significantly-incomplete features on by default.
May 22 2018, 9:26 PM · Restricted Project
rjmccall added a comment to D47103: Implement strip.invariant.group.

The changes to Clang generally seem reasonable, but I think you should split them into a separate commit from the commit that adds the intrinsic itself.

May 22 2018, 8:58 PM
rjmccall added a reviewer for D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type: akyrtzi.

CC: Argyrios for the USR question.

May 22 2018, 8:51 PM · Restricted Project
rjmccall added a comment to D47108: [CodeGenCXX] Add -fforce-emit-vtables.

I thought we already had places in Sema that marked inline virtual methods as used, instantiated templates, etc. for devirtualization purposes when optimization was enabled. Did we rip that out?

May 22 2018, 8:48 PM
rjmccall accepted D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably.

LGTM, thanks!

May 22 2018, 1:05 PM
rjmccall added a comment to D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably.

I like this approach a lot.

May 22 2018, 11:25 AM
rjmccall accepted D47099: Call CreateTempAllocaWithoutCast for ActiveFlag.

Thanks, LGTM.

May 22 2018, 1:42 AM

May 21 2018

rjmccall added inline comments to D47099: Call CreateTempAllocaWithoutCast for ActiveFlag.
May 21 2018, 2:16 PM
rjmccall added a comment to D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.

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.

May 21 2018, 11:28 AM · Restricted Project
rjmccall added a comment to D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.

This was approved by the Objective-C language group as a default-off warning.

May 21 2018, 12:52 AM · Restricted Project

May 20 2018

rjmccall accepted D46052: GNUstep Objective-C ABI version 2.

Thanks, my comments seem to all be addressed.

May 20 2018, 10:21 PM

May 19 2018

rjmccall added a comment to D47099: Call CreateTempAllocaWithoutCast for ActiveFlag.

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.

May 19 2018, 2:16 PM

May 18 2018

rjmccall accepted D47096: CodeGen: block capture shouldn't ICE.

LGTM.

May 18 2018, 9:23 PM
rjmccall added a comment to D47099: Call CreateTempAllocaWithoutCast for ActiveFlag.

Maybe there should just be a method that makes a primitive alloca without the casting, and then you can call that in CreateTempAlloca.

May 18 2018, 9:22 PM
rjmccall added a comment to D47096: CodeGen: block capture shouldn't ICE.

Test case should go in test/CodeGenCXX. Also, there already is a blocks.cpp there.

May 18 2018, 9:20 PM
rjmccall added a comment to D47096: CodeGen: block capture shouldn't ICE.
In D47096#1105374, @jfb 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?

May 18 2018, 7:34 PM
rjmccall added inline comments to D47092: downgrade strong type info names to weak_odr linkage.
May 18 2018, 7:27 PM
rjmccall added a comment to D47096: CodeGen: block capture shouldn't ICE.

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?

May 18 2018, 4:43 PM
rjmccall added a comment to D47092: downgrade strong type info names to weak_odr linkage.

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?

May 18 2018, 4:28 PM
rjmccall added inline comments to D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages..
May 18 2018, 4:20 PM
rjmccall added a comment to D47092: downgrade strong type info names to weak_odr linkage.

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?

May 18 2018, 4:13 PM
rjmccall added a comment to D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.

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.

May 18 2018, 3:22 PM · Restricted Project
rjmccall added a comment to D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.

It's waiting on a discussion that's happening pretty slowly, sorry. I know this is frustrating.

May 18 2018, 1:16 PM · Restricted Project

May 17 2018

rjmccall added a comment to D46685: [CodeGen] Disable structor optimizations at -O0.

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.

May 17 2018, 10:07 PM
rjmccall added a comment to D45898: [SemaCXX] Mark destructor as referenced .

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.

May 17 2018, 10:02 PM

May 16 2018

rjmccall accepted D45900: CodeGen: Fix invalid bitcast for lifetime.start/end.

LGTM.

May 16 2018, 8:24 PM