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

New attribute: Tag_RISCV_reserved_register #195

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kito-cheng
Copy link
Collaborator

Motivation

Tag_RISCV_reserved_register used for record reserved register information,
desiged for resolve checking compatibility between ABI subvariants like
overlay ABI.

Meaning

This attribute is a bit-vector of reserved register, but it can also
representation as a register list, syntax below:

RESERVED_REGS := '{' REG_LIST '}'

REG_LIST      := REG_LIST ',' REG_LIST
               | REG_RANGE
               | REG

REG_RANGE     := REG '-' REG

REG           := <register-name> | <abi-register-name> # e.g. x10, t3, f10 or fa2

For example: x6, x7, x8 and f10 are reserved, then the value of
Tag_RISCV_reserved_register is 0x400000001c1, and it also could be represent
as {x6, x7, x8, f10} or {x6-x8, f10}.

Merge Rule

It will report errors if link object files with different
Tag_RISCV_reserved_register values, but allowed link with object with and
without Tag_RISCV_reserved_register value, the final value will take from the
object which has set Tag_RISCV_reserved_register.

@jim-wilson
Copy link
Collaborator

The stuff about the register lists is a little confusing, as the encoding chosen for Tag_RISCV_reserved_register requires a bit vector. So I think you might be talking about the acceptable assembly input, or objdump/readelf output. But not the encoding in the object file. This is a little unclear the way it is worded. Maybe add something about human readable input/output can use the alternate syntax. Also not clear if this is the right place to document assembly syntax. Maybe that should be in the asm manual?

Otherwise this looks reasonable to me.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jul 6, 2021

Given custom extensions start at 3072 this doesn't make sense in the current form IMO.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jul 6, 2021

Also, a file without the tag being merged with a file with the tag is just asking for trouble.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jul 6, 2021

Your grammar also parses ambiguously as you have REG_LIST on either side of the comma, even though the "operator" is associative so it doesn't really matter. I'd be inclined to express it as:

RESERVED_REGS ::= '{' REG_LIST '}'

REG_LIST      ::= REG_RANGE ',' REG_LIST
                | REG_RANGE

REG_RANGE     ::= REG '-' REG
                | REG

REG           ::= <register-name> | <abi-register-name> # e.g. x10, t3, f10 or fa2

and maybe also add | '{' '}' to RESERVED_REGS if the empty list is allowed to be written and/or encoded.

riscv-elf.md Outdated
@@ -1043,6 +1043,7 @@ Tag_RISCV_unaligned_access | 6 | uleb128 | Indicates whet
Tag_RISCV_priv_spec | 8 | uleb128 | Indicates the major version of the privileged specification.
Tag_RISCV_priv_spec_minor | 10 | uleb128 | Indicates the minor version of the privileged specification.
Tag_RISCV_priv_spec_revision | 12 | uleb128 | Indicates the revision version of the privileged specification.
Tag_RISCV_reserved_register | 14 | uleb128 | Indicates the extra reserved register information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be plural

@kito-cheng
Copy link
Collaborator Author

Also, a file without the tag being merged with a file with the tag is just asking for trouble.

My thought is provide some flexible for human written asm file, but I admit that could be a trouble, I think in future we might add an extra asm directive to make this thing easier (e.g. .option overlay).

I'll update that later to remove the exception merge rule.

@kito-cheng
Copy link
Collaborator Author

@jim-wilson

The stuff about the register lists is a little confusing, as the encoding chosen for Tag_RISCV_reserved_register requires a bit vector. So I think you might be talking about the acceptable assembly input, or objdump/readelf output. But not the encoding in the object file. This is a little unclear the way it is worded. Maybe add something about human readable input/output can use the alternate syntax. Also not clear if this is the right place to document assembly syntax. Maybe that should be in the asm manual?

Thanks for the review comment, split human readable input/output part to asm manual sounds good to me, we don't have any word for .attribute for asm manual yet, it's a good opportunity to fill out that :P...

@edward-jones
Copy link

I have a patch for LLVM which implements this here: D113890

This is needed in order to reserve registers for use by the overlay engine.

@edward-jones
Copy link

I have a branch of my own which tries to incorporate the feedback from here (also a rebase). I can turn this into a pull request of its own if that's best?

https://github.com/edward-jones/riscv-elf-psabi-doc/tree/erj-reserved-register-attr

I agree that the assembler syntax/human readable format should be in the assembler manual.

As for the merging of objects files with/without the tag, I think best to disallow mixing entirely and then an option could be added later to explicitly supress that warning in cases where you're deliberately linking object files with different tags.

- Motivation:

  Tag_RISCV_reserved_register used for record reserved register information,
  desiged for resolve checking compatibility between ABI subvariants like
  overlay ABI.

- Meaning:

  This attribute is a bit-vector of reserved register, syntax for
  assembly language please refer
  https://github.com/riscv-non-isa/riscv-asm-manual .

- Merge Rule:

  It will report errors if link object files with different
  Tag_RISCV_reserved_register values, but allowed link with object with and
  without Tag_RISCV_reserved_register value, the final value will take from the
  object which has set Tag_RISCV_reserved_register.
- The attribute name is plural now, Tag_RISCV_reserved_registers
- Make the distinction between the encoded value and the human
  readable syntax a bit clearer.
- Remove ambiguity from the grammar for the tag based on feedback.
- Add possibility for an empty register list to the grammar
- Remove the exception allowing merging of two objects files if
  one includes the tag and the other doesn't
@kito-cheng
Copy link
Collaborator Author

Changes:

@edward-jones
Copy link

Can I ask what the path forward is for this. In the psABI call someone (I can't remember who?) suggested that there may be a simpler solution for the overlay system, but I don't think they elaborated on what the solution was?

@kito-cheng
Copy link
Collaborator Author

IIRC Anders Berg (from IAR) said, he will give move detail next time.

@oferShinaar
Copy link

The PR is important for Overlay, but it is a generic solution for saving registers + the option to validate register usage between libs on link time.
We can say it is an alternative solution to clang -ffixed-xx (which btw is what we are currently using in Overlay)

We don't see any correlation between this PR and the "helper functions" or in a future new ABI.
Users can choose to use RISCV reserved register tag on any ABI.

@kito-cheng
Copy link
Collaborator Author

Hi Ofer:

The policy for psABI is we won't merge anything new stuffs before releasing 1.0, however I guess that might take another 2 or 3 month due to the release process, so I think maybe we could branch out 1.0, and then keep including new stuffs for master branch, however that need to discuss with @jrtc27 and @cmuellner in next Toolchain & Runtime TS Bi-Weekly Meeting (2/14).

@oferShinaar
Copy link

oferShinaar commented Feb 8, 2022 via email

@jrtc27
Copy link
Collaborator

jrtc27 commented Feb 8, 2022

Temporarily forking the document risks leading to confusion, but even if that weren't an issue, handling things other than those for 1.0 takes away precious time from getting the release out the door. Let's just focus on what we need to rather than getting distracted by future things, especially those that are quite subjective like this.

@kito-cheng
Copy link
Collaborator Author

@jrtc27 :
I guess @oferShinaar is want to make sure the name and number is reserved and make sure their own implementation won't break (too much) in future.

So maybe we could give a label (maybe pre-approved-new-tag?) for that to mark that is kind of pre-approved for the tag name and tag number? and defer the merge until 1.0 is released.

@jrtc27
Copy link
Collaborator

jrtc27 commented Feb 14, 2022

Reserving relocation numbers requires reaching a consensus that the set of relocations is correct. I do not want to waste what little time I have pushing forward non-1.0 things.

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.

5 participants