rjmccall added a comment. In D102015#2748441 <https://reviews.llvm.org/D102015#2748441>, @efriedma wrote:
> In D102015#2743634 <https://reviews.llvm.org/D102015#2743634>, @rjmccall > wrote: > >> Objective-C object types also have aggregate evaluation kind. Those can >> only be used as value types in an old, fragile ObjC ABI, but I believe we >> haven't yet formally decided to remove support for that. Fortunately, >> however, there's a much better simplification available: this >> `hasAggregateEvaluationKind(Ty)` call is literally doing nothing that >> couldn't just be a `getAs<RecordType>()` call, and then we don't need to >> worry about it. > > If you're confident that only RecordTypes need to be destroyed, sure. Well, they're the only thing for which `isParamDestroyedInCallee()` is defined. If we generalize that in the future, I think the code in your patch will be obvious enough how to modify. In D102015#2748595 <https://reviews.llvm.org/D102015#2748595>, @efriedma wrote: > In D102015#2748441 <https://reviews.llvm.org/D102015#2748441>, @efriedma > wrote: > >>> ...I'm confused about why this code is doing what it's doing with cleanups, >>> though. Why does it only apply when the parameter is indirect? I believe >>> `isParamDestroyedInCallee()` can apply to types that are passed in other >>> ways, so where we pushing the destructor for those, and why isn't it good >>> enough to handle this case as well? >> >> Objects with a non-trivial destructor end up indirect anyway under the >> normal ABI rules. Not sure how it interacts with trivial_abi; I'll look >> into it. > > Figured it out. "isIndirect()" here doesn't mean the same thing that it > means in ABIArgInfo. If `hasScalarEvaluationKind(Ty)` is false, the caller > ensures the value is "isIndirect()", i.e. on the stack. It doesn't matter if > the value was actually passed in registers. Ugh. Well, that's not great, but yeah, I agree we don't need to mess with that right now. Thanks for checking it out. In D102015#2749212 <https://reviews.llvm.org/D102015#2749212>, @efriedma wrote: >> Using _Atomic for C structs with non-trivially destructible fields currently >> doesn't work, so maybe that should just be disallowed. > > I'd prefer to disallow _Atomic on all types that aren't trivially > destructible, yes. Not impossible to support, of course, but I don't really > see the value. I agree that disallowing this is the right way to go. There are non-trivial C structs that can support atomic operations easily enough, but the ones with ARC strong references specifically aren't among them. I guess your test case is testing both the parameter and argument code. The delegate-arg code should be testable with an override thunk with a big atomic parameter, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102015/new/ https://reviews.llvm.org/D102015 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
