O1 #205
O1 #205
Conversation
| @@ -1982,9 +1982,10 @@ void AsmPrinter::emitFunctionBody() { | |||
| break; | |||
| default: | |||
|
|
|||
| // Note that a patchpoint call, unlike a stackmap call, does actually | |||
There was a problem hiding this comment.
I think I understand this comment in the context of this PR but after this PR is merged, this comment will probably bemuse readers, who won't have the context that makes it make sense.
I wonder if it should be reworded to something vaguely along the lines of "Stackmaps and statepoints have to XYZ. However, patchpoints -- even though they're similar to stackmaps and statepoints -- don't have to XYZ because ABC."?
There was a problem hiding this comment.
Maybe: "We don't need emit labels for STACKMAP since this intrinsic doesn't codegen a call. Patchpoints however, do emit calls, and so need to be included".
By the way, STATEPOINT also emits calls. But we don't use them.
There was a problem hiding this comment.
Does that mean that the STATEPOINT line in the conditional should also be removed?
There was a problem hiding this comment.
I think we can remove it. Since we don't use STATEPOINT it doesn't affect us, but technically I think it being there is incorrect. At least from what I can tell from https://llvm.org/docs/Statepoints.html#safepoint-semantics-verification.
There was a problem hiding this comment.
I improved the comment in 4068ee0, but I hadn't seen the discussion on statepoint at that time.
@ptersilie would you mind tackling this, if any change is required.
There was a problem hiding this comment.
Let's just rip out the STATEPOINT line now; if we need it, we'll surely see it in some tests.
|
Ready to squash? |
|
Please squash. |
|
squashed! |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
9c95422 is the fix. OK to squash it in? |
|
Please squash. |
It was talking about the old design that used a struct for live variables.
|
squashed. |
Companion to ykjit/yk#1400