This is an archive of the discontinued LLVM Phabricator instance.

[Coroutine] Remain alignment information when merging frame variables
ClosedPublic

Authored by ChuanqiXu on Jan 17 2021, 7:23 PM.

Details

Summary

This is to address bug48712.
The solution in this patch is that when we want to merge two variable a into the storage frame of variable b only if the alignment of a is multiple of b.
There may be other strategies. But now I think they are hard to handle and benefit little. Or we can implement them in the future.

Test-plan: check-llvm

Diff Detail

Event Timeline

ChuanqiXu created this revision.Jan 17 2021, 7:23 PM
ChuanqiXu requested review of this revision.Jan 17 2021, 7:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2021, 7:23 PM

@231084fg may I ask why you remove the reviews for the patch?

MyDeveloperDay added a subscriber: MyDeveloperDay.EditedJan 19 2021, 1:18 AM

@231084fg may I ask why you remove the reviews for the patch?

This person is user since only 15th Jan, I'm beginning to think they may be malicious!

@hans , @rsmith , @benhamilton anyone mind if I disable this users account? they seem to be randomly going through phabricator making changes which seem to be unrelated.

Their email is also not verified.

@231084fg may I ask why you remove the reviews for the patch?

This person is user since only 15th Jan, I'm beginning to think they may be malicious!

@hans , @rsmith , @benhamilton anyone mind if I disable this users account? they seem to be randomly going through phabricator making changes which seem to be unrelated.

Their email is also not verified.

Thanks for explaining! I visited the his profile. It is really confusing.

Thanks for explaining! I visited the his profile. It is really confusing.

I also noticed the same when the clang-format project got removed (archived), along with a whole load of other projects. From what I can tell this person seems to be making changes which from what I can tell I don't understand why.

Thanks for explaining! I visited the his profile. It is really confusing.

I also noticed the same when the clang-format project got removed (archived), along with a whole load of other projects. From what I can tell this person seems to be making changes which from what I can tell I don't understand why.

Yes, his behavior is really confusing. And I wonder it may suggest the access control system of Phabricator need to do better. It is surprising that an unverified email could remove a subproject.

jmorse accepted this revision.Jan 19 2021, 2:28 AM

LGTM with comments inline. While it's a simple fix, I'm not especially familiar with coroutines, so it'd be nice if someone else could chime in before landing.

llvm/lib/Transforms/Coroutines/CoroFrame.cpp
608–612

This doesn't need to be a lambda, it's only called once

1145
This revision is now accepted and ready to land.Jan 19 2021, 2:28 AM

@231084fg may I ask why you remove the reviews for the patch?

This person is user since only 15th Jan, I'm beginning to think they may be malicious!

@hans , @rsmith , @benhamilton anyone mind if I disable this users account? they seem to be randomly going through phabricator making changes which seem to be unrelated.

Their email is also not verified.

I've gone ahead and disabled this user.

lxfind added inline comments.Jan 19 2021, 8:50 AM
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
1132

Is this change unintentional? It seems to be AI and Orig is the same thing. What's the reason to use AI below instead of just Orig?

ChuanqiXu added inline comments.Jan 19 2021, 5:48 PM
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
608–612

I wonder if the logic could be more clear when we wrap the code in lambda.

1132

Oh, yes, it is unintentional change. I would revert this change.

ChuanqiXu updated this revision to Diff 317736.Jan 19 2021, 5:58 PM

Remove unintentional change.

Refine typos.