Stream: git-cranelift

Topic: cranelift / PR #1373 Properly preserve and restore CFA st...


view this post on Zulip GitHub (Feb 01 2020 at 15:28):

yurydelendik opened PR #1373 from fde_multi_ret to master:

Fixes #1372 (discussion there)

The patch properly arranges frame layout info commands for restore and preserve state. Also, it properly serializes them in into FDE.

view this post on Zulip GitHub (Feb 01 2020 at 15:28):

yurydelendik requested iximeow for a review on PR #1373.

view this post on Zulip GitHub (Feb 02 2020 at 06:48):

yurydelendik updated PR #1373 from fde_multi_ret to master:

Fixes #1372 (discussion there)

The patch properly arranges frame layout info commands for restore and preserve state. Also, it properly serializes them in into FDE.

view this post on Zulip GitHub (Feb 02 2020 at 06:49):

yurydelendik updated PR #1373 from fde_multi_ret to master:

Fixes #1372 (discussion there)

The patch properly arranges frame layout info commands for restore and preserve state. Also, it properly serializes them in into FDE.

view this post on Zulip GitHub (Feb 03 2020 at 20:07):

iximeow submitted PR Review.

view this post on Zulip GitHub (Feb 03 2020 at 20:07):

iximeow submitted PR Review.

view this post on Zulip GitHub (Feb 03 2020 at 20:07):

iximeow created PR Review Comment:

move isn't needed on this closure.

view this post on Zulip GitHub (Feb 03 2020 at 20:07):

iximeow created PR Review Comment:

Was there an error in inserting Preserve/Restore outside insert_common_epilogue, or is this just a refactor to keep everything in insert_common_epilogue? Regardless, I don't have a great feel for Layout's API, and last_ebb is much better than the above "is there an instruction in the next basic block" that was there before.

view this post on Zulip GitHub (Feb 03 2020 at 20:07):

iximeow created PR Review Comment:

I have a loose preference for making the right vec in the first place rather than conditionally removing an element especially since is_last is an argument now. But, lacking a benchmark I'm not sure if it really matters in release builds.

view this post on Zulip GitHub (Feb 03 2020 at 20:07):

iximeow created PR Review Comment:

I think it's worth noting that it's correct to not see additional CFA directives after restore_state here, because the function exit past this point is a trap. I mention here for two reasons:

view this post on Zulip GitHub (Feb 03 2020 at 21:10):

yurydelendik updated PR #1373 from fde_multi_ret to master:

Fixes #1372 (discussion there)

The patch properly arranges frame layout info commands for restore and preserve state. Also, it properly serializes them in into FDE.

view this post on Zulip GitHub (Feb 03 2020 at 21:13):

yurydelendik created PR Review Comment:

There was an error in previous logic: it was inserting Preserve after the CallFrameAddressAt, which made useless. The same issue was with Restore -- it was inserted too late.

view this post on Zulip GitHub (Feb 03 2020 at 21:13):

yurydelendik submitted PR Review.

view this post on Zulip GitHub (Feb 03 2020 at 21:14):

yurydelendik edited PR Review Comment.

view this post on Zulip GitHub (Feb 03 2020 at 21:14):

yurydelendik requested iximeow for a review on PR #1373.

view this post on Zulip GitHub (Feb 03 2020 at 22:07):

iximeow submitted PR Review.

view this post on Zulip GitHub (Feb 03 2020 at 22:07):

iximeow created PR Review Comment:

oh! yes, I see the errors now.

view this post on Zulip GitHub (Feb 03 2020 at 22:08):

iximeow submitted PR Review.

view this post on Zulip GitHub (Feb 03 2020 at 22:08):

iximeow merged PR #1373.


Last updated: Dec 23 2024 at 13:07 UTC