- User Since
- Jan 18 2013, 11:30 AM (296 w, 4 d)
Fri, Sep 21
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.
Thu, Sep 20
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.
Wed, Sep 19
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.
Tue, Sep 18
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".
Sun, Sep 16
It might help if you're more specific about whose review you're asking for.
Mon, Sep 10
Wed, Sep 5
Committed as r341491.
Committed as r341489.
Tue, Sep 4
Fri, Aug 31
Thu, Aug 30
Wed, Aug 29
LGTM, with one minor suggestion.
Tue, Aug 28
It says the type of the assignment expression, not the type of the LHS.
Mon, Aug 27
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.
Aug 24 2018
Aug 23 2018
Aug 22 2018
Please read the developer policy: https://llvm.org/docs/DeveloperPolicy.html
Aug 21 2018
Aug 20 2018
Aug 17 2018
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 16 2018
Aug 15 2018
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 14 2018
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.
Sure, that seems like a reasonable optimization.
Aug 13 2018
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.
Aug 10 2018
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 9 2018
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?
Aug 8 2018
Alright, LGTM, at least until we have that backend support you mentioned.
Aug 7 2018
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 6 2018
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.
Aug 3 2018
Aug 2 2018
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.