In D52344#1242451, @kristina wrote:

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.

In D52339#1242241, @aaron.ballman wrote:

In D52339#1242202, @erik.pilkington wrote:

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.

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.

In D52268#1241793, @riccibruno wrote:

In D52268#1241033, @rjmccall wrote:

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.

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.

Thanks, LGTM.

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.

In D52200#1239007, @aaronpuchert wrote:

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.

In D51789#1238410, @ldionne wrote:

In D51789#1238396, @rjmccall wrote:

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?

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".

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

LGTM.

Committed as r341491.

Committed as r341489.

LGTM.

In D51200#1223768, @kuhar wrote:

In D51200#1223752, @rsmith wrote:

+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)?

In D51290#1219217, @agozillon wrote:

In D51290#1214739, @rjmccall wrote:

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?

LGTM.

LGTM, with one minor suggestion.

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

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.

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

In D44539#1208780, @QF5690 wrote:

In D44539#1207982, @rjmccall wrote:

LGTM.

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

LGTM.

LGTM.

In D50527#1206460, @erik.pilkington wrote:

Ping!

In 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?

In D50616#1203772, @lebedev.ri wrote:

In D50616#1203751, @rjmccall wrote:

In 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.

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.

LGTM.

In D50616#1203692, @ebevhan wrote:

In 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 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.

In D50616#1203446, @leonardchan wrote:

In 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?

Okay.

In 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.

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.

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.

LGTM.

Sure, that seems like a reasonable optimization.

In 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.

In 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.

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?

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.

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.

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

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.

LGTM.

Thanks, LGTM.

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.

Hey, still working on this?

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

In 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?

LGTM.

In 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.

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?

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

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

Sure, that's fine.

LGTM.

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.