-
-
Notifications
You must be signed in to change notification settings - Fork 272
[LLVM 21] fix calls to llvm.lifetime.start
#4979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+88
−25
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The missing
elsehere compared to my commit now means that this line above has no more effect for IR-void-typed vars;irLocal->valueis immediately overwritten below with a dummy void-alloca (probably promoted to some dummy i8 storage). And now gets DI infos.Edit: And additionally caused you to come up with a weird logic for the lifetime-start annotation apparently. So please just try my commit.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted wr.t. DI infos, fixed that. Nope:
still fails with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding back in the
ifthen causes it to pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternately if (AFAIU) the entire point of these lifetime annotations is to aid debugging, then we could restrict them to only user variables (i.e. ig more all compiler generated ones beginning with
__, e.g.__copytempwhich in addition to__tmpfordtorand__slseems to cover all the bases).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well IIRC, Phobos couldn't be compiled successfully in #4990 after cherry-picking an earlier version of this PR's commit, but applying both linked commits I linked made it work with LLVM 21 and older versions at least. I think I understood what you were after with the earlier unrelated refactoring (combining the DI and lifetime-start conditions to non-IR-void-typed vars only, without the
realAllocahelper var and a 2nd void check), that made sense; it's just that the early return instead of anelsebranch caused skipping some stuff at the end of the function.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that the
sretNRVO case is handled just above in an earlierif:ldc/gen/llvmhelpers.cpp
Lines 887 to 900 in b5f7bb0
That's where the lval is set to the sret argument instead of an alloca. And where no lifetime-start annotation is added.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but as your interesting reduced test case shows, that's not enough. In-place construction can replace a temporary alloca later with an sret argument, as the lvalue of a variable:
ldc/gen/toir.cpp
Lines 3002 to 3015 in b5f7bb0
According to the
-vvlog, this is what happens in the lastmemoizeExprreturn statement:So
__copytmp3first gets an alloca, and then that lval is IR-replaced with the sret argument later, incl. for the lifetime-start call. Your diagnosis might be right wrt.__copytmp3apparently not being the frontend-NRVO variable directly; the return expression in theif (__ctfe)block might break it (as the NRVO var must normally be returned in every return statement of the function).Edit: But there are always going to be remaining non-NRVO cases. So for these lifetime annotations, we might have to skip this in-place construction of sret values from non-NRVO temporaries (edit: oh well, the extra memcpy and different address might break code depending on RVO though...). Or scan the existing uses, to remove the lifetime intrinsic calls (possibly missing the lifetime end calls if they are pushed later on...), before replacing all uses.
Edit2: I think what would probably be best is to pre-set the lval (i.e.,
getIrLocal(vd, true)->value) of the temporary to the sret argument intoInPlaceConstruction(), before evaluating the comma-expression. [Incl. emitting DI there, asDtoVarDeclaration()wouldn't do it anymore due toisIrLocalCreated(vd).] Instead of replacing all IR uses after a normal evaluation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, an
if(false) return ...;also breaks NRVO. I opened a DMD issue for this.The removal of
isRealAllocawas mostly to try to use the type system to fix/make sure I wasn't doing anything stupid with it, but it seems that thereplaceAllUsesWithis what is causing it to change type from anAllocaInsttoArgument. Thank you for finding that, I thought I was going mad!It seems to me that the easiest solution then would be to strip the calls to
@llvm.lifetimeprior to thereplaceAllUsesWith, though I don't quite understand what you are trying to say in yourEdit2.Anyway I've added two of the reductions to the test suite (there is another one with
__tmpfordtorthat I ned to re-run). I'm going away for the weekend, so if you feel like implementing any of your suggestions, please do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good find!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've tried: #5015. I'm pretty sure I've added that replace-all-uses-with hack, so only fair to clean it up now. :)