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

Add a new directive .variant_cc for variant calling conventions. #64

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

Hsiangkai
Copy link
Contributor

No description provided.

@ebahapo
Copy link
Contributor

ebahapo commented May 12, 2021

Instead of a new directive, perhaps a new .type argument would make more sense, since functions typically take a .type to be specified as function symbols.

@Hsiangkai
Copy link
Contributor Author

Instead of a new directive, perhaps a new .type argument would make more sense, since functions typically take a .type to be specified as function symbols.

ARM defines a new directive, .variant_pcs, for the purpose. Should we extend the syntax of .type?

@kito-cheng
Copy link
Collaborator

@jim-wilson @jrtc27

Copy link
Collaborator

@kito-cheng kito-cheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Collaborator

@jim-wilson jim-wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't invent anything new if we don't have to. .variant_cc is consistent with the aarch64 port. This looks good to me.

@kito-cheng
Copy link
Collaborator

kito-cheng commented Aug 12, 2021

@asb @jrtc27 @luismarques @MaskRay

This PR is related to Calling conventions for the lazily bound functions. riscv-non-isa/riscv-elf-psabi-doc#190, used to mark a symbol is STO_RISCV_VARIANT_CC, and do not lazy binding for this symbol.

I guess there is no much controversy here since the solution just same as AArch64, but I would like to got some LGTM from LLVM community.

@asb
Copy link
Contributor

asb commented Aug 19, 2021

@asb @jrtc27 @luismarques @MaskRay

This PR is related to Calling conventions for the lazily bound functions. riscv/riscv-elf-psabi-doc#190, used to mark a symbol is STO_RISCV_VARIANT_CC, and o not lazy binding for this symbol.

I guess there is no much controversy here since the solution just same as AArch64, but I would like to got some LGTM from LLVM community.

I've got no objections, but I'd defer to @MaskRay on this one.

@MaskRay
Copy link

MaskRay commented Aug 19, 2021

(@asb Thanks for the trust:) )

LGTM (I don't have github permission to approve).

Using a new directive (not reusing .type) has the following advantages beside following aarch64's lead:

  • .type is for st_type. st_other is orthogonal to st_type. (gnu_unique_object was an unfortunate anti-example.)
  • compilers don't set .type for undefined symbols, but .variant_cc is needed at reference site for correctness.
  • avoid overriding the generic .type with arch-specific semantics.

I assume that we cannot reuse .variant_pcs because PCS is an aarch64 term...
A new directive theoretically allows extension without introducing user confusion, though it may be hard to think how .variant_cc could be extended in the future. (One possibility is to allow .variant_cc a, b, c but aarch64 doesn't support it, either; and this doesn't seem too useful anyway.)

The subject should be changed. "bind now " seems outdated. Ideally just mention the directive name .variant_cc in the subject. It makes log digging easier in the future.

@kito-cheng
Copy link
Collaborator

@jim-wilson @aswaterman could you merge this PR? I believe we reached a consensus between LLVM and GNU toolchain folks, and the psabi spec merged too, riscv-non-isa/riscv-elf-psabi-doc#190

@jrtc27
Copy link
Contributor

jrtc27 commented Nov 1, 2021

Commits should be squashed and reworded to not talk about bind now

@Hsiangkai Hsiangkai changed the title Add a new directive for specifying bind now semantic for a symbol. Add a new directive .variant_cc for specifying variant calling convention semantic for a symbol. Nov 9, 2021
@Hsiangkai Hsiangkai changed the title Add a new directive .variant_cc for specifying variant calling convention semantic for a symbol. Add a new directive .variant_cc for variant calling conventions. Nov 9, 2021
@Hsiangkai
Copy link
Contributor Author

Commits are squashed and the title is changed. Sorry for the late update.

@jim-wilson jim-wilson merged commit b01f833 into riscv-non-isa:master Nov 9, 2021
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.

None yet

7 participants