User Details
- User Since
- Jan 18 2013, 11:30 AM (417 w, 2 d)
Sat, Jan 16
Clang is trying to avoid using non-byte-multiple integer types for memory accesses in LLVM IR, whereas Swift isn't. If you take your test case and make the field non-atomic, you'll see that: Clang is accessing the field as an i8 and Swift is accessing it as an i1. So there's a deeper mismatch here which in practice works out because there's no actual difference in memory access width between those types.
Fri, Jan 15
Hmm. The way you'd test it on the Clang side would be to just write a test case that passes such an aggregate and tests that it's passed as an i8. But I'm not sure it's at all unreasonable to pass this as an i1 rather than an i8; seems like something that Swift should handle correctly in its call-lowering code.
Wed, Jan 13
I'm sorry that it's taking a while to find time to review the implementation; I'll try to get to it this week.
Thu, Jan 7
Yeah, that's definitely an LLVM test.
Okay, LGTM.
Wed, Jan 6
LGTM
I'm not calling Wine a niche use-case, I'm calling this feature a niche use-case. The lack of this feature has not blocked Wine from being a successful project. The feature has to stand on its own and be more broadly useful than the momentary convenience of a few developers.
Tue, Jan 5
Are you committed to the name __ibm128? I think a name that says something about floating point would be better than just a company name and a number. "double double" is the common name for this format pretty much everywhere, and while I certainly would *not* suggest actually spelling it double double (i.e. with repeated type specifiers0), you could certainly use something like __doubledouble.
I do feel like this is a terrible idea, sorry. It's a *very* niche use case to be dedicating a new compiler feature to, and it's a feature that demands a lot from the internal compiler architecture in ways that other features don't.
LGTM
People can't actually just copy/paste from the comment anyway because of the comment characters on the line, and I don't think anyone will misunderstand the example if the backslash is missing. It's silly that GCC has a problem with this, but since it does, let's just work around the problem and drop the backslash rather than having one comment block in the header use a different style.
Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.
Wed, Dec 23
Functionally LGTM. Minor suggestion.
Tue, Dec 22
Dec 17 2020
LGTM
byval actually means it'll be passed on the stack, not as a pointer, so yes, that's a real change in conventions from an indirect argument.
Oh yes, somehow I overlooked that, sorry.
Dec 16 2020
I think that as long as the class leaves a copy/move constructor defaulted, there's no need for a new trivial_abi attribute.
Please test that there's actually an object declared and that it's not *just* a tag declaration.
I'm not entirely sure what you're still waiting for, really. It's a big patch with a lot of diffuse responsibilities, but you've gotten sign-off from individual people across at least most of it. Do you feel like there's something significant that hasn't been reviewed?
Dec 10 2020
Seems fine to me.
Dec 9 2020
Dec 8 2020
Your patch description doesn't mention the changes to the inliner.
Dec 4 2020
There is no such thing as an object "teleporting" in C++. Objects are always observed in memory with a specific address. When an argument is passed in registers, its address can be observed to be different on both sides, and that is not permitted; there must be some operation that creates the object at the new address and destroys it at the old. That operation is a destructive move (which I certainly did not originate as a term for this), or what your proposal calls a relocation (which may be a more palatable term for the C++ committee).
Dec 3 2020
Dec 2 2020
Dec 1 2020
That's an excellent example to study. The fundamental question we are looking to answer is whether a type representationally depends on its address. Absent the trivial_abi attribute, the criterion for knowing that it does not is whether it is possible to copy or move it trivially. If it is not possible to copy or move the type at all, it is not possible to copy or move it trivially. So S0 may representationally depend on its address, and we should ignore/diagnose trivial_abi on aggregates containing an S0.
Nov 30 2020
This seems like a good change, but we should make sure we test cases that may previously have been covered by the implicit deletion of all copy/move constructors. That may just be a matter of auditing tests.
Functionally LGTM. I don't know if 11 is still taking changes, but if it is, you have code-owner approval.
Nov 18 2020
I assume this has always been taken care of properly in the backend, so this is just a fix for va_arg treatment? If so, LGTM.
Nov 17 2020
We don't usually add known-broken tests like this before a fix, as opposed to just landing them as part of the fix, but if you have a good reason to do so it's okay.
Nov 13 2020
LGTM
Nov 12 2020
LGTM
Nov 11 2020
LGTM.
Probably should be pluralized for consistency, fast-honor-pragmas, but yeah, that's fine with me.
Oh, unfortunately llvm.coro.async.continuation wouldn't be able to just return the continuation function pointer; there would have to be a llvm.coro.async.get_continuation_function intrinsic. But we do something similar with the alloc intrinsics.
I see. This LGTM as an immediate fix, then.
Nov 10 2020
What is this function being inlined? We expect the frontend to emit a thunk that glues things together?
fast-strict? Sounds sort of oxymoronic, but it's fast while also being strict about honoring pragmas. I don't have any great ideas here.
Nov 9 2020
This is great work, thank you.
I don't quite understand the need to inline here.
Alright.
Nov 6 2020
LGTM
Nov 5 2020
Okay. It sounds like strict compatibility with GCC implies ignoring pragmas in fast, and that's what we're most concerned with, since this is originally a GCC option. So the only question I have now is whether faststd is the best name for this. Does anyone want to suggest an alternative?
Nov 3 2020
Looks good to me as a first pass.
Hmm. Do we actually want this behavior of fast overriding pragmas? What do other compilers do here? It might be reasonable to just treat this as a bug.
Nov 2 2020
I agree this is useful. However, you need to update the manual to cover faststd.
Pointer authentication can also make structs non-trivial to copy.
I'm not sure how overlap-aware our packing is, but yeah, I'm fine with removing this.
Oct 30 2020
I may not have been clear. I'm not saying SourceLocation is a meaningful concept in the driver. I'm saying that if you generalize the concept of "source location" to "location in the input", there is a clear analogue in the driver (namely, a position in the argument list), and the only reason this isn't passed down to the driver and used in diagnostics is that we don't have the ability to express that today to the diagnostic engine. So instead of trying to extract out a part of the diagnostics engine that will work without any concept of source locations, you should be trying to parameterize the diagnostics engine so that it can work with an arbitrary external concept of source locations, and then the driver can use a different kind of source location than the main compiler and everything is fine.
Oct 29 2020
Alright, LGTM.
It sounds like what you want is a diagnostic library that's almost completely abstracted over the kinds of entities that can be stored in a diagnostic, including the definition of a source location. I don't think that's incompatible with this patch; there's no need to suggest reversion.
Oct 28 2020
Patch generally looks good. Minor complaint about how you're using notes.
Oct 27 2020
LGTM. Untyped pointers can't come soon enough.
Oct 26 2020
Argh, sorry! I meant to say "I have *no* objections".
I have objections to the code change here. I'll leave the conceptual question to other people interested in the HIP toolchain.
Oct 23 2020
Yes, there are no generically available libcalls for atomic float math -- but that's okay -- let LLVM handle transform into a cmpxchg loop when required.
LGTM.
Oct 22 2020
Oh, I see, sorry.
Mangling more calling conventions when mangling function types in Itanium (except at the top level) is the right thing to do. There's already a place to do so in the mangling. We just haven't done this yet because a lot of those calling convention are supported by other compilers, so we need to coordinate. So you need to check out what other compilers do (GCC, ICC) and imitate them if they're already setting a reasonable default; otherwise, I think there's a standard algorithm for generating these.
Oct 21 2020
Okay. Well, it does seem to me that we need to be considering lifetimes. If we can also optimize when we don't have lifetime information and the variable doesn't escape, that's great, but we often have pretty good lifetime information that we can use. And in optimized builds we'll usually have already eliminated allocas that don't escape, so the remaining allocas are very likely to have all escaped.
We do not actually support allocation failure for a lot of things around blocks. I don't think the copy-helper functions even have a way to propagate out a failure in copying a field. I have never seen any code in the wild that would handle Block_copy returning a null pointer. Effectively it is assumed to not happen.
There's an existing StackLifetime analysis that does some of this work and also considers the lifetime intrinsics; can we take advantage of that?
Of course, that wouldn't solve the general problem that the block might be getting used before its capture is fully initialized, but that's a general problem with uses within initializers in C.
It's not optimal, but an alternative would be to force the variable to the heap immediately rather than waiting for a potential block copy. The variable would actually be uninitialized during its "copy", so we'd need to give it a trivial copy helper. But once the variable's been moved to the heap, it's never copied again, so that's fine.
Oct 19 2020
Okay. This LGTM if you feel that the operator forwarders is the better approach.
Yeah, it might be reasonable to just do that on targets where the defaults are different (just MSVC, I think?) and otherwise preserve the method's calling convention.
Oct 16 2020
We can't have it always match, that would require us to reject the conversion to a bare function-pointer type, which is required by the standard.
Okay, thanks. This LGTM.
No, if you put a calling convention on a lambda and then convert it to a function pointer, you almost certainly want that CC to be honored.
I agree that it seems reasonable to preserve an explicit CC from the lambda on the converted function pointer.
Even if we decide to change that recommendation, this is not the place to elaborate on the advice. We have an entire section of the developer's manual dedicated to providing guidance on choosing data structures, and we should not duplicate it clumsily and in part out here.
llvm::SmallVector isn't, like, otherwise worse than std::vector, so that you should only consider it over std::vector when the inline-capacity optimization is highly valuable. It does have some specific drawbacks, but we go into this in more detail in the programmer's manual. I think "prefer llvm::SmallVector" is fine general advice for this paragraph.
Oct 15 2020
LGTM