[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

Tue Sep 19 11:11:37 PDT 2023

rjmccall added a comment.

> That's better than the status quo (no lifetime markers at all) but still suboptimal, and I don't think it will solve the particular case I care about (a particular Linux kernel driver written in C which is passing many aggregates by value).

Ah, all in the same full-expression?  Then yeah, my idea wouldn't be good enough to optimize your situation.

> Do I want to create a flag for that? Not particularly; it does mean supporting BOTH codegen patterns; I'd rather have a flag to control the optimization or not do it at all rather than support 2 different optimizations.

The received wisdom here is that it's very helpful to have a flag because it lets users test the effect of different changes independently.  Users mostly don't live on trunk, and LLVM releases are roughly every six months, so when users get a new compiler it's going to have at least six months of accumulated changes in it.  If a project breaks with a new compiler, it's really great to be able to isolate which change is causing the problem without having to rebuild the compiler.

The fact that we have dynamic checking of this from ASan does help a lot, though.

> Personally, I feel that "we found a buggy example of code we broke, we fixed it. Let's reland it and tell people to be careful when upgrading." But I haven't been around long enough to know what's the precedent here for "aggressive optimizations." Do you feel strongly that we should not just reland this, @rjmccall (or anyone else)?

That several things broke immediately after commit does suggest that there are quite a bit more project bugs out there waiting to blow up.  I think having a dedicated flag for just this optimization is probably a good idea in the short term.

> One thing I noticed, simply printing the expression when we were about to add the lifetime cleanup; I noticed some examples would trigger on CXXConstructExpr, CXXTemporaryObjectExpr, and CXXOperatorCallExpr. Those feel very C++ specific; I do wonder if we could do something differently for C, but I think we should not since as @rjmccall said "we'd have a consistent rule for all types and targets" which IMO is nicer+simpler.

Well, if the language rules are different, they're different.  (More on that in a second.)  As far as I know, the lifetime of a parameter variable in C is never specified as outliving the call, and the entire concept of a full-expression is a C++ism.  We could do something more aggressive in C.

> Converting the above widget example to C, I noticed that we're also missing the lifetime markers on compound literals!

Yeah, so this is probably because the lifetime of a compound literal is somewhat surprising in C: it's actually the containing block scope.  We could emit lifetime markers, but we'd have to make sure we emitted them in the right place.  In pure C, I think this is only detectable if the user actually takes the address of the literal; unlike C++, C doesn't implicitly expose the address of temporaries or have destructors that allow their lifetime to be directly tested.  But in ObjC, you could have a compound literal with weak/strong references in it, which arguably are required to be destroyed at the close of the enclosing block.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094