Skip to content

Place intrinsics in individual object files #349

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 10, 2020
Merged

Place intrinsics in individual object files #349

merged 1 commit into from
Apr 10, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Apr 10, 2020

No description provided.

@bjorn3
Copy link
Member

bjorn3 commented Apr 10, 2020

I think you will need to add #[inline] to all the functions wrapped by the intrinsics to ensure that they are compiled in the same codegen unit as the intrinsic.

@tmiasko
Copy link
Contributor Author

tmiasko commented Apr 10, 2020

The interaction with other attributes is not ideal. I had to disable attribute expansions in win64_128bit_abi_hack, this seems fine now, but could be a footgun in a future.

@bjorn3 So far I didn't found it necessary for release builds (based on nm and objdump -r --disassemble). Is it different for you?

@alexcrichton
Copy link
Member

Should we perhaps go ahead and just place all intrinsics in their own codegen unit?

And yes it is not necessary to use #[inline] due to ThinLTO.

@tmiasko
Copy link
Contributor Author

tmiasko commented Apr 10, 2020

I added #[inline] on undefined symbols from nm to get similar results regardless of ThinLTO.

I cross-validated partitioning with libgcc.a installed on my system. They match with default features, and there is a small mismatch when c feature is enabled. I don't feel much need to change things further, unless there is some specific target that would like me to test against.

@alexcrichton
Copy link
Member

I don't think there's any realistic need for more than one symbol per CGU, even if libgcc does something similar. I also think it would be better to automatically place everything with one-symbol-per-object rather than having to manually annotate each function with a new attribute.

@tmiasko tmiasko changed the title Place some intrinsics in individual compilation units Place intrinsics in individual object files Apr 10, 2020
@alexcrichton
Copy link
Member

Is the #[inline] necessary here for all these functions? I would say that we should probably avoid adding that if we can, only adding it if we profile at some point and see that LLVM isn't doing great-enough inlining.

@tmiasko
Copy link
Contributor Author

tmiasko commented Apr 10, 2020

The #[inline] is necessary to get code that doesn't have large amount of trivial indirections for non-ThinLTO builds. I can drop those changes if you want.

@alexcrichton
Copy link
Member

I agree yeah that if ThinLTO is disabled it's a problem, but I think ThinLTO is enabled everywhere, so I'm not sure it's a case we need to worry about?

@tmiasko
Copy link
Contributor Author

tmiasko commented Apr 10, 2020

In context of official builds, I would expect this to be the case. Hard to say how it used more generally. In any case, we can revisit this aspect later if necessary. I removed inlining changes for now.

@alexcrichton alexcrichton merged commit 2541f27 into rust-lang:master Apr 10, 2020
@alexcrichton
Copy link
Member

Ok I've published 0.1.27 with this now too

@tmiasko tmiasko deleted the compilation-unit branch April 13, 2020 15:52
tgross35 added a commit to tgross35/compiler-builtins that referenced this pull request Feb 23, 2025
Migrate types from a macro to a trait
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants