rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (296 w, 4 d)

Recent Activity

Fri, Sep 21

rjmccall added a reviewer for D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.: theraven.

Would you be okay with me renaming cfstring to .cfstring for ELF so it's in line with ELF section naming conventions (I'm not entirely sure if that could cause issues with ObjC stuff)? And yes I think it's best to avoid that code-path altogether if it turns out to be a constant as that's likely from the declaration of the type, I'll write a FileCheck test in a moment and check that I can apply the same logic to ELF aside from the DLL stuff as CoreFoundation needs to export the symbol somehow while preserving the implicit extern attribute for every other TU that comes from the builtin that CFSTR expands to. Is what I'm suggesting making sense? If not, let me know, I may be misunderstanding what's happening here.

Fri, Sep 21, 1:43 PM
rjmccall added inline comments to D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`..
Fri, Sep 21, 12:24 PM
rjmccall added a comment to D52339: Support enums with a fixed underlying type in all language modes.

From the last line in the paper, it seems that C++ compatibility is a goal of the paper (or at least a consideration). We should probably think about this if/when the final wording gets accepted though.

Agreed. WG14's charter explicitly prefers compatibility with C++ when possible.

The part that I wasn't quite sure on were the type constraints in the proposal. C++ allows any integral type and ignores cv qualifiers, but the proposal lists specific types and doesn't discuss qualifiers. By my reading, this is code allowed in C++ but prohibited in the proposed wording:

enum E : const int32_t {
  One
};

(Because the type is int32_t and is cv-qualified.) However, it's possible that's an oversight rather than an intentional design. I'll bring it up with Clive to see, but perhaps we can spot other divergences and can provide him with a list of concerns on the implementation side.

Fri, Sep 21, 11:06 AM
rjmccall accepted D52352: llvm-diff: Fix crash on anonymous functions.

LGTM. And yeah, I don't have a better recommendation in the short term. Long-term, maybe we can try to match them up by uses or types.

Fri, Sep 21, 11:03 AM
rjmccall accepted D52268: [AST] Squeeze some bits in LinkageComputer.

LinkageComputer isn't actually persisted anywhere, right? And there's maybe one computer active at once? So this compression is theoretically saving one pointer of stack space but forcing a bunch of bit-manipulation every time these fields are accessed.

It is not persisted but this saves one pointer per entry in the map. Another factor is that hashing a pair involves hashing
each component and then combining the result, which is comparatively much more expansive than just hashing a PointerIntPair,
which involves only a shift and a xor. The field storing the LVComputationKind is never directly read but only used to differentiate
various kinds of computations in the map. I went back and instrumented the lookup function LinkageComputer::lookup with rdtsc,
and (with all the usual caveats about microbenchmarks and rdtsc) I get that this cuts the number of ticks spent inside lookup
from about 8e6 to 3.5e6. Now of course taking a step back this represent only milliseconds and is firmly in the category of
"way to small to bother", but now we might as well do it.

Fri, Sep 21, 10:59 AM · Restricted Project

Thu, Sep 20

rjmccall added a comment to D52268: [AST] Squeeze some bits in LinkageComputer.

LinkageComputer isn't actually persisted anywhere, right? And there's maybe one computer active at once? So this compression is theoretically saving one pointer of stack space but forcing a bunch of bit-manipulation every time these fields are accessed.

Thu, Sep 20, 1:29 PM · Restricted Project
rjmccall accepted D52267: [AST] Various optimizations + refactoring in DeclarationName(Table).
Thu, Sep 20, 11:46 AM · Restricted Project

Wed, Sep 19

rjmccall accepted D52271: [Sema] Ensure that we retain __restrict qualifiers when substituting a reference type..

Thanks, LGTM.

Wed, Sep 19, 10:55 PM
rjmccall added inline comments to D52271: [Sema] Ensure that we retain __restrict qualifiers when substituting a reference type..
Wed, Sep 19, 9:42 PM
rjmccall added a comment to D52267: [AST] Various optimizations + refactoring in DeclarationName(Table).

Conceptually, this change looks great. And it should be fine to require extra alignment on IdentifierInfo on 32-bit hosts; I doubt that will have measurable impact.

Wed, Sep 19, 9:36 PM · Restricted Project

Tue, Sep 18

rjmccall added a comment to D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate.

I think it should be possible to get rid of self-> in the warning message if we want to, after all this-> is omitted in C++ as well.

Tue, Sep 18, 7:26 PM
rjmccall added a comment to D51789: [clang] Add the exclude_from_explicit_instantiation attribute.

That may work for libc++'s purposes, but it's clearly inappropriate as a compiler rule. There are good reasons why something with hidden visibility would need to be explicitly instantiated.

I take your word for it, but I can't think of any example. For my education, do you have a specific example in mind?

Tue, Sep 18, 10:27 AM
rjmccall added inline comments to D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate.
Tue, Sep 18, 8:26 AM
rjmccall added a comment to D51789: [clang] Add the exclude_from_explicit_instantiation attribute.

That may work for libc++'s purposes, but it's clearly inappropriate as a compiler rule. There are good reasons why something with hidden visibility would need to be explicitly instantiated. For many programmers, hidden visibility means "this is private to my library", not "this is actually public to my library, but I'm walking an ABI tightrope".

Tue, Sep 18, 8:12 AM

Sun, Sep 16

rjmccall added a comment to D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part.

It might help if you're more specific about whose review you're asking for.

Sun, Sep 16, 11:54 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D51990: [DebugInfo] Fix emitting of bit offset for ObjC.
Sun, Sep 16, 11:49 PM · debug-info

Mon, Sep 10

rjmccall accepted D51084: Implement -Watomic-implicit-seq-cst.

LGTM.

Mon, Sep 10, 9:02 AM

Wed, Sep 5

rjmccall closed D51426: [Sema] Prohibit function-scope compound literals with address spaces..

Committed as r341491.

Wed, Sep 5, 12:24 PM
rjmccall committed rL341491: Forbid address spaces on compound literals in local scope..
Forbid address spaces on compound literals in local scope.
Wed, Sep 5, 12:24 PM
rjmccall committed rC341491: Forbid address spaces on compound literals in local scope..
Forbid address spaces on compound literals in local scope.
Wed, Sep 5, 12:23 PM
rjmccall committed rC341489: Add -Wobjc-property-assign-on-object-type..
Add -Wobjc-property-assign-on-object-type.
Wed, Sep 5, 12:05 PM
rjmccall committed rL341489: Add -Wobjc-property-assign-on-object-type..
Add -Wobjc-property-assign-on-object-type.
Wed, Sep 5, 12:05 PM
rjmccall closed D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.

Committed as r341489.

Wed, Sep 5, 12:05 PM · Restricted Project
rjmccall accepted D51564: Distinguish `__block` variables that are captured by escaping blocks from those that aren't.

LGTM.

Wed, Sep 5, 11:15 AM

Tue, Sep 4

rjmccall added a comment to D51200: Introduce per-callsite inline intrinsics.

+rjmccall for CodeGen changes

@rsmith @rjmccall
I have one high-level question about the CodeGen part that I wasn't able to figure out: is it possible to bail out of custom CodeGen in CGBuiltin and somehow say: emit whatever is inside (IE treat the builtin call node as a no-op)?

Tue, Sep 4, 6:26 PM · Restricted Project
rjmccall added inline comments to D51290: Dependent Address Space qualifier removal and reapplication bug fix.
Tue, Sep 4, 10:36 AM

Fri, Aug 31

rjmccall added inline comments to D51564: Distinguish `__block` variables that are captured by escaping blocks from those that aren't.
Fri, Aug 31, 11:44 PM
rjmccall added inline comments to D51084: Implement -Watomic-implicit-seq-cst.
Fri, Aug 31, 11:15 PM

Thu, Aug 30

rjmccall added inline comments to D51290: Dependent Address Space qualifier removal and reapplication bug fix.
Thu, Aug 30, 4:20 PM
rjmccall added a comment to D51290: Dependent Address Space qualifier removal and reapplication bug fix.

The TreeTransform change obviously looks good. The template-argument-deduction change seems at least a little suspicious; I don't know that ordering is a problem, but I'm not sure why this only moves fast qualifiers when there are other qualifiers potentially there. Also, TAD can be performed on dependent types when partially-ordering templates, which can happen with e.g. class template partial specializations, so you should make sure that this code handles that well.

You should pretty much always add Richard as a reviewer when making a tricky change in the template system.

Thank you very much for the quick reply and addition of Richard as a reviewer, I'll be sure to add him in the future when it concerns templates.

Yes you're correct, I was focusing too much on a concise fix here I think, I should have considered other qualifiers. I'm not entirely clued up on fast qualifiers however; which qualifiers would not be considered fast if you don't mind me asking?

Thu, Aug 30, 12:07 PM
rjmccall accepted D51426: [Sema] Prohibit function-scope compound literals with address spaces..

LGTM.

Thu, Aug 30, 1:52 AM
rjmccall added inline comments to D51426: [Sema] Prohibit function-scope compound literals with address spaces..
Thu, Aug 30, 1:15 AM

Wed, Aug 29

rjmccall accepted D51426: [Sema] Prohibit function-scope compound literals with address spaces..

LGTM, with one minor suggestion.

Wed, Aug 29, 9:50 AM

Tue, Aug 28

rjmccall added a comment to D51084: Implement -Watomic-implicit-seq-cst.

It says the type of the assignment expression, not the type of the LHS.

Tue, Aug 28, 6:22 PM
rjmccall added inline comments to D51084: Implement -Watomic-implicit-seq-cst.
Tue, Aug 28, 5:05 PM

Mon, Aug 27

rjmccall added a reviewer for D51290: Dependent Address Space qualifier removal and reapplication bug fix: rsmith.

The TreeTransform change obviously looks good. The template-argument-deduction change seems at least a little suspicious; I don't know that ordering is a problem, but I'm not sure why this only moves fast qualifiers when there are other qualifiers potentially there. Also, TAD can be performed on dependent types when partially-ordering templates, which can happen with e.g. class template partial specializations, so you should make sure that this code handles that well.

Mon, Aug 27, 10:50 AM

Aug 24 2018

rjmccall added inline comments to D51084: Implement -Watomic-implicit-seq-cst.
Aug 24 2018, 5:40 PM

Aug 23 2018

rjmccall added inline comments to D51084: Implement -Watomic-implicit-seq-cst.
Aug 23 2018, 10:09 AM

Aug 22 2018

rjmccall added a comment to D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.

Please read the developer policy: https://llvm.org/docs/DeveloperPolicy.html

Aug 22 2018, 1:09 AM · Restricted Project
rjmccall added a comment to D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.

LGTM.

Thanks! What I should do next? Haven't found any info in docs about it :)

Aug 22 2018, 12:33 AM · Restricted Project

Aug 21 2018

rjmccall accepted D51025: [CodeGen] Fix handling of variables captured by a block that is nested inside a lambda.

LGTM.

Aug 21 2018, 1:32 PM
rjmccall accepted D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.

LGTM.

Aug 21 2018, 11:29 AM · Restricted Project

Aug 20 2018

rjmccall accepted D50527: [Parser] Support alternative operator token keyword args in Objective-C++.

Ping!

Aug 20 2018, 2:07 PM
rjmccall added a comment to D50616: [Fixed Point Arithmetic] FixedPointCast.

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?

Aug 20 2018, 12:29 PM · Restricted Project

Aug 17 2018

rjmccall added a comment to D50616: [Fixed Point Arithmetic] FixedPointCast.

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.

Aug 17 2018, 1:58 PM · Restricted Project
rjmccall added a comment to D50616: [Fixed Point Arithmetic] FixedPointCast.

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.

Aug 17 2018, 1:14 PM · Restricted Project
rjmccall accepted D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression.

LGTM.

Aug 17 2018, 12:59 PM
rjmccall added inline comments to D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression.
Aug 17 2018, 12:22 PM
rjmccall added a comment to D50616: [Fixed Point Arithmetic] FixedPointCast.

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.

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.

Aug 17 2018, 2:21 AM · Restricted Project

Aug 16 2018

rjmccall added a comment to D50616: [Fixed Point Arithmetic] FixedPointCast.

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?

Aug 16 2018, 11:19 PM · Restricted Project
rjmccall added inline comments to D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression.
Aug 16 2018, 11:05 PM
rjmccall accepted D50783: [CodeGen] Merge identical block descriptor global variables.
Aug 16 2018, 11:00 PM
rjmccall added a comment to D50783: [CodeGen] Merge identical block descriptor global variables.

Okay.

Aug 16 2018, 4:11 PM

Aug 15 2018

rjmccall added a comment to D50783: [CodeGen] Merge identical block descriptor global variables.

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.
Aug 15 2018, 1:20 PM
rjmccall added inline comments to D50616: [Fixed Point Arithmetic] FixedPointCast.
Aug 15 2018, 1:13 PM · Restricted Project
rjmccall added a comment to D50631: [AST] Stuff more data into FunctionTypeBitfields.

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.

Aug 15 2018, 12:25 PM · Restricted Project

Aug 14 2018

rjmccall added a comment to D21767: Fix instantiation of friend function templates.

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.

Aug 14 2018, 2:21 PM
rjmccall accepted D50715: [AST] Store the OwnedTagDecl as a trailing object in ElaboratedType..

LGTM.

Aug 14 2018, 11:07 AM · Restricted Project
rjmccall added a comment to D50630: [AST] Update/correct the static_asserts for the bit-fields in Type.

Sure, that seems like a reasonable optimization.

Aug 14 2018, 11:04 AM · Restricted Project

Aug 13 2018

rjmccall added a comment to D50630: [AST] Update/correct the static_asserts for the bit-fields in Type.

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.

Aug 13 2018, 6:02 PM · Restricted Project
rjmccall added a comment to D50630: [AST] Update/correct the static_asserts for the bit-fields in Type.

@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.
Aug 13 2018, 2:23 PM · Restricted Project
rjmccall accepted D50596: [HIP] Make __hip_gpubin_handle hidden to avoid being merged across different shared libraries.
Aug 13 2018, 12:48 PM
rjmccall added a comment to D21767: Fix instantiation of friend function templates.

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?

Aug 13 2018, 12:29 PM
rjmccall added a comment to D50630: [AST] Update/correct the static_asserts for the bit-fields in Type.

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.

Aug 13 2018, 12:23 PM · Restricted Project
rjmccall added a comment to D50631: [AST] Stuff more data into FunctionTypeBitfields.

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.

Aug 13 2018, 12:20 PM · Restricted Project
rjmccall accepted D50559: [gnu-objc] Make selector order deterministic..

Thanks. I appreciate the fact that you spelled it all out in the test, too.

Aug 13 2018, 12:03 PM
rjmccall added inline comments to D50616: [Fixed Point Arithmetic] FixedPointCast.
Aug 13 2018, 12:02 PM · Restricted Project

Aug 10 2018

rjmccall added a comment to D50559: [gnu-objc] Make selector order deterministic..

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.

Aug 10 2018, 1:02 PM

Aug 9 2018

rjmccall accepted D50144: Add Windows support for the GNUstep Objective-C ABI V2..

LGTM.

Aug 9 2018, 10:33 PM
rjmccall accepted D50152: [CodeGen] Merge equivalent block copy/helper functions.

Thanks, LGTM.

Aug 9 2018, 10:28 PM
rjmccall added inline comments to D50152: [CodeGen] Merge equivalent block copy/helper functions.
Aug 9 2018, 8:06 PM
rjmccall added a comment to D50527: [Parser] Support alternative operator token keyword args in Objective-C++.

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.

Aug 9 2018, 4:22 PM
rjmccall added inline comments to D50152: [CodeGen] Merge equivalent block copy/helper functions.
Aug 9 2018, 4:19 PM
rjmccall added a comment to D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute.

Hey, still working on this?

Aug 9 2018, 12:56 AM · Restricted Project

Aug 8 2018

rjmccall added inline comments to D50152: [CodeGen] Merge equivalent block copy/helper functions.
Aug 8 2018, 8:04 PM
rjmccall added inline comments to D50152: [CodeGen] Merge equivalent block copy/helper functions.
Aug 8 2018, 5:22 PM
rjmccall added inline comments to D50152: [CodeGen] Merge equivalent block copy/helper functions.
Aug 8 2018, 12:59 PM
rjmccall added inline comments to D50144: Add Windows support for the GNUstep Objective-C ABI V2..
Aug 8 2018, 12:52 PM
rjmccall accepted D50436: Correctly initialise global blocks on Windows..

Alright, LGTM, at least until we have that backend support you mentioned.

Aug 8 2018, 12:35 PM
rjmccall added inline comments to D50144: Add Windows support for the GNUstep Objective-C ABI V2..
Aug 8 2018, 12:33 AM

Aug 7 2018

rjmccall added inline comments to D50144: Add Windows support for the GNUstep Objective-C ABI V2..
Aug 7 2018, 10:31 PM
rjmccall added a comment to D50152: [CodeGen] Merge equivalent block copy/helper functions.

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?

Aug 7 2018, 9:42 PM
rjmccall accepted D50278: [Sema] Fix for crash on conditional operation with address_space pointer.

LGTM.

Aug 7 2018, 11:34 AM · Restricted Project
rjmccall added a comment to D50278: [Sema] Fix for crash on conditional operation with address_space pointer.

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.

Aug 7 2018, 11:28 AM · Restricted Project
rjmccall added a comment to D50152: [CodeGen] Merge equivalent block copy/helper functions.

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?

Aug 7 2018, 11:19 AM

Aug 6 2018

rjmccall added inline comments to D50278: [Sema] Fix for crash on conditional operation with address_space pointer.
Aug 6 2018, 3:51 PM · Restricted Project
rjmccall added a comment to D50152: [CodeGen] Merge equivalent block copy/helper functions.

You might want to ask Richard on IRC if there are caveats when using that for these purposes.

Aug 6 2018, 2:34 PM
rjmccall added inline comments to D50152: [CodeGen] Merge equivalent block copy/helper functions.
Aug 6 2018, 2:31 PM
rjmccall added inline comments to D50278: [Sema] Fix for crash on conditional operation with address_space pointer.
Aug 6 2018, 1:38 PM · Restricted Project
rjmccall added inline comments to D50152: [CodeGen] Merge equivalent block copy/helper functions.
Aug 6 2018, 1:23 PM
rjmccall added inline comments to D50278: [Sema] Fix for crash on conditional operation with address_space pointer.
Aug 6 2018, 1:18 PM · Restricted Project
rjmccall added a comment to D50278: [Sema] Fix for crash on conditional operation with address_space pointer.

I would expect this to replace the existing warning, not to appear together with it.

Aug 6 2018, 1:16 PM · Restricted Project
rjmccall accepted D49952: Check for NULL Destination-Type when creating ArrayConstant.

Sure, that's fine.

Aug 6 2018, 1:13 PM

Aug 3 2018

rjmccall accepted D50286: avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls.

LGTM.

Aug 3 2018, 6:23 PM
rjmccall added inline comments to D49952: Check for NULL Destination-Type when creating ArrayConstant.
Aug 3 2018, 6:14 PM
rjmccall added inline comments to D50286: avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls.
Aug 3 2018, 6:11 PM

Aug 2 2018

rjmccall added inline comments to D50152: [CodeGen] Merge equivalent block copy/helper functions.
Aug 2 2018, 9:53 PM
rjmccall added a comment to D49403: More aggressively complete RecordTypes with Function Pointers.

Sorry, let me be more precise. The semantics of LLVM IR are that the element type of a pointer matters when doing specific operations on pointers: loads, stores, GEPs, calls, and so on. Many of these operations have been at least partially updated — in some combination of the APIs for constructing them and the IR assembly listings — to require the "element" type to be given separately, but that's not really instrumental here. What's instrumental is that, for almost all of these operations, the only thing that matters about the element type is its basic layout. For example, it is perfectly legal in IR to load a float by loading any type that happens to be the same size as a float and then bitcasting that value to float — well, maybe it's not okay to load it as a struct with internal padding, because I think the padding bits might take on unspecified values, but anything else. It is also legal in IR to do pointer arithmetic by using any combination of GEPs that happens to yield the right offset. The only pointer operations where the specific details of the element type actually have semantic weight beyond their layout are calls.

Aug 2 2018, 4:18 PM
rjmccall added inline comments to D50152: [CodeGen] Merge equivalent block copy/helper functions.
Aug 2 2018, 4:13 PM
rjmccall added inline comments to D50003: Sema: Fix explicit address space cast involving void pointers.
Aug 2 2018, 3:57 PM