Skip to content
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

Merge identical block copy/destroy helpers and descriptors #66358

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nocchijiang
Copy link

This patch is trying to merge identical block copy/destroy helpers and descriptors which is similar to the patch for Clang.

Due to insufficient knowledge about the compiler, I don't know how to properly encode the capture type for copy/destroy helpers. For now the encoding is hardcoded.

I am seeking help on the forum, unfortunately there has been no response. So I decide to create the pull request to draw some attention on GitHub to anyone familiar with this field.

@nocchijiang nocchijiang force-pushed the nocchijiang/merge_identical_block_metadata branch 2 times, most recently from edd0c11 to 1a172e4 Compare August 2, 2023 09:31
@nocchijiang
Copy link
Author

Since no one is responding to the post or this PR I decide to finish it on my own.

3 things I'm not sure if I've done right:

  1. Is the "initialization" behavior needed in the encoding?
  2. I did the encoding in a top-down approach with mininal implementation to pass the existing test. I prefer Clang's bottom-up approach but it seems impossible to do so in Swift.
  3. I reused the scalarToString for the encoding of ReferenceCounting enum which is a bit too verbose. I think refCountString() was a good candidate but it is refactored into a compact binary encoding.

@jckarter could you please have a look at this PR and the things I mentioned above?

@nocchijiang
Copy link
Author

@rjmccall Joe haven't responded for 2 weeks, could you please take a look at the PR?

@rjmccall
Copy link
Contributor

I'm sorry that this has been sitting without review for so long; it slipped through my net.

The huge number of new calls to llvm_unreachable gives me pause here. If this is only supported for a tiny number of types, it should be written in a way that will reliably fall back and use unmerged helpers if there are types it doesn't support. You can do that either by defaulting the new TypeInfo methods or by just recognizing the specific types you handle.

It would be really nice if this could be done in a way that was more generally useful. I know @jckarter has been looking at the idea of a little VM for performing value operations. I assume that's built on a notion of recursively identifying interesting values in a layout. If that can just be abstracted a tiny bit, so that we can intercept pieces of abstract information like "there's a ref-counted object at offset 24", then we could write your thing on top of it by mangling all those individual pieces of information instead of needing yet another recursive descent through TypeInfo implementations.

@nocchijiang nocchijiang force-pushed the nocchijiang/merge_identical_block_metadata branch from 1a172e4 to c21222d Compare September 6, 2023 13:33
@nocchijiang
Copy link
Author

@rjmccall Thanks for reviewing! In the latest patch I removed all calls to llvm_unreachable and wrote default implementation as you suggested. Could you please review it again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants