Stream: git-cranelift

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

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.

GitHub (Feb 01 2020 at 15:28):

yurydelendik requested iximeow for a review on PR #1373.

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.

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.

GitHub (Feb 03 2020 at 20:07):

iximeow submitted PR Review.

GitHub (Feb 03 2020 at 20:07):

iximeow submitted PR Review.

GitHub (Feb 03 2020 at 20:07):

iximeow created PR Review Comment:

move isn't needed on this closure.

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.

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.

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:

• double-checking my own understanding :)
• I think it'd be good to have a comment about this, for someone reading these tests in the future.

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.

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.

GitHub (Feb 03 2020 at 21:13):

yurydelendik submitted PR Review.

GitHub (Feb 03 2020 at 21:14):

yurydelendik edited PR Review Comment.

GitHub (Feb 03 2020 at 21:14):

yurydelendik requested iximeow for a review on PR #1373.

GitHub (Feb 03 2020 at 22:07):

iximeow submitted PR Review.

GitHub (Feb 03 2020 at 22:07):

iximeow created PR Review Comment:

oh! yes, I see the errors now.

GitHub (Feb 03 2020 at 22:08):

iximeow submitted PR Review.

GitHub (Feb 03 2020 at 22:08):

iximeow merged PR #1373.

Last updated: Nov 22 2024 at 16:03 UTC