search

riscv-non-isa / riscv-asm-manual

RISC-V Assembly Programmer's Manual
https://jira.riscv.org/browse/RVG-4
Creative Commons Attribution 4.0 International
1.44k stars 238 forks source link

Proposal: Extend .option directive for control enabled extensions on specific code region #67

Closed kito-cheng closed 1 year ago

kito-cheng commented 3 years ago

Changes:

This commit extend .option diretive with new keyword:

Enable and/or disable specific ISA extensions for following code regions, but without changing the arch attribute, that means it won't raise the minimal execution environment requirement, so the user should take care to protect execution of the code regions around .option push/.option arch/.option pop. A typical use case is is with ifunc, e.g. the libc is built with rv64gc, but a few functions like memcpy provide two versions, one built with rv64gc and one built with rv64gcv, and then select between them by ifunc mechanism at run-time. However we don't want to change the minimal execution environment requirement to rv64gcv, since the rv64gcv version will be invoked only if the execution environment supports the vector extension, so the minimal execution environment requirement still is rv64gc.

Example:

.attribute arch, "rv64imafdc"
 # You can only use instruction from i, m, a, f, d and c extensions.
memcpy_general:
    add     a5,a1,a2
    beq     a1,a5,.L2
    add     a2,a0,a2
    mv      a5,a0
.L3:
    addi    a1,a1,1
    addi    a5,a5,1
    lbu     a4,-1(a1)
    sb      a4,-1(a5)
    bne     a5,a2,.L3
.L2:
    ret 

.option push    # Push current option to stack.
.option arch +v # Enable vector extension, we can use any instruction in imafdcv extension.
memcpy_vec:
    mv a3, a0
.Lloop:
    vsetvli t0, a2, e8, m8, ta, ma
    vle8.v v0, (a1)
    add a1, a1, t0
    sub a2, a2, t0
    vse8.v v0, (a3)
    add a3, a3, t0
    bnez a2, .Lloop
    ret
.option pop   # Pop current option from stack, restore the enabled ISA extension status to imafdc.

Notes:

jrtc27 commented 3 years ago

Why is this a new directive rather than just a .option? We already have .option push/.option pop, all you need is a .option to change the arch string like now we already have rvc/norvc. Perhaps that should just be generalised to (no)rv$arch?

jrtc27 commented 3 years ago

i.e.:

.attribute arch, "rv64imafdc"
memcpy_general:
    add     a5,a1,a2
    beq     a1,a5,.L2
    add     a2,a0,a2
    mv      a5,a0
.L3:
    addi    a1,a1,1
    addi    a5,a5,1
    lbu     a4,-1(a1)
    sb      a4,-1(a5)
    bne     a5,a2,.L3
.L2:
    ret

.option push
.option rvv
memcpy_vec:
    mv a3, a0
.Lloop:
    vsetvli t0, a2, e8, m8, ta, ma
    vle8.v v0, (a1)
    add a1, a1, t0
    sub a2, a2, t0
    vse8.v v0, (a3)
    add a3, a3, t0
    bnez a2, .Lloop
    ret
.option pop    # Restore the enabled ISA extension status to imafdc.

This is cleaner, more consistent and avoids adding more than one way to enable/disable RVC.

jrtc27 commented 3 years ago

Looks OK with some English improvements. Only useful if both clang and GNU as support it at roughly the same time, and will require configure checks in GNU tools before using, and whatever the clang equivalent is.

The assembler is integrated (well, it doesn't even bother to serialise and re-parse textual assembly) so normally no such concern exists. When there is enough interest in supporting old GNU assemblers with the integrated assembler turned off there's -fbinutils-version that has to be supplied by the user. But configure-time does not make sense for Clang as there's very little distinction between native and cross-compilation beyond the fact it picks a default triple, and every build contains all backends by default, and you're not going to have an assembler for every possible supported target present when you build Clang.

kito-cheng commented 3 years ago

Why is this a new directive rather than just a .option? We already have .option push/.option pop, all you need is a .option to change the arch string like now we already have rvc/norvc. Perhaps that should just be generalised to (no)rv$arch?

Reason why define new directive rather than extend rvc/norvc scheme.

Here is an another issue there: should we have negative option for .push_arch/.pop_arch? e.g. .push_arch -c, .push_arch -v

jrtc27 commented 3 years ago

Why is this a new directive rather than just a .option? We already have .option push/.option pop, all you need is a .option to change the arch string like now we already have rvc/norvc. Perhaps that should just be generalised to (no)rv$arch?

Reason why define new directive rather than extend rvc/norvc scheme.

  • Able to included the version info in the option.

.option rvv1p0

  • Able to enable more than one extension at a time.

.option rvv1p0, rvb0p93, or I guess you can .option rvb0p93_v1p0 etc and allow an arch "substring", both work. But even without those options, having to have multiple lines is hardly a big deal... how often do you need to enable more than one extension? And how often is that being done by hand rather than just auto-generated by a loop in the compiler? It's a non-issue.

  • Implementation is simpler, it won't mixed with existing .option implementation.

Disagree. Uniformity is simpler. Parsing this is trivial and generalises the code that's already there. Having two ways to do the same thing confuses users, looks ugly and means you have multiple copies of similar code in your assembler.

Here is an another issue there: should we have negative option for .push_arch/.pop_arch? e.g. .push_arch -c, .push_arch -v

Yes. My solution supports that.

jrtc27 commented 3 years ago

Other than aesthetics and arguments about simplicity/consistency, there is a very real concern I have about having two different push/pop mechanisms. What does:

.attribute arch, rv32i2p0
.push_arch m
.option rvc
.pop_arch

mean? Is RVC enabled or disabled at the end of that? Or:

.attribute arch, rv32i2p0
.option push
.push_arch m
.option pop

Does that have RVM enabled or disabled at the end? If you make the push and pop parts of .push_arch and .pop_arch be the same as .option then they are completely redundant and should not exist beyond the "set the arch string" part (i.e. pop_arch goes away and push_arch only modifies the current arch string), but then push_arch is a bad name for that part. If you don't make them equivalent and try and maintain two separate stacks then that becomes a complete tangled nightmare both in the implementation and in the conceptual model. Thus, whatever you name the thing, it needs to ultimately be equivalent to .option (no)rv$arch, and so might as well be called that for consistency with (no)rvc rather than some kind of .modify_arch.

kito-cheng commented 3 years ago

@jrtc27 thanks your example, that's good point, let me write down second version for this proposal.

Nelson1225 commented 3 years ago

I already have the first version of .push_arch and .pop_arch in GNU binutils, but haven't written the test cases yet. Nelson1225/riscv-binutils-gdb@8e2483f

The implementation and assembly syntax of .option rvc; .option norvc; and .push_arch c2p0; .pop_arch are different, though they all need to be stored in the stack. I agree with @jrtc27 that we should use the same format and way to do this stuff, so seems that there are some issues as follows,

  1. Extend .option rvc that can have versions. for example, 
    
.attribute arch, rv32i2p0
.option push  /* backup rv32i2p0.  */
.option rvc2p0  /* added c2p0, rv32i2p0_c2p0.  */
.option rvc3p0  /* updated c3p0, rv32i2p0_c3p0.  */
.option norvc  /* removed c, rv32i2p0.  */
.option pop  /* restored rv32i2p0.  */
So the format `.option rv<extension><version>` is good, but `.option norv<extension><version>` will complicate the usage, so I suggest do not use versions in the `.option norv<extension>`

2. Not only the standard extensions can be pushed and popped, so according to the above formats.

.option rv and .option norv

.attribute arch, rv32i2p0_m2p0 .option push / backup rv32i2p0. / .option rvc2p0 / added c2p0, rv32i2p0_c2p0. / .option rvzfh0p1 / added zfh0p1, rv32i2p0_c2p0_zfh0p1. / .option rvxvendor1p0 / added xvendor1p0, rv32i2p0_c2p0_zfh0p1_xvendor1p0. / .option pop / restored rv32i2p0. /

Although `rv` prefix looks redundant, but we need to consider the compatibility of rvc and norvc.

3. Should we still support the following format?

.option rv, rv, ... .option norv, norv, ...

kito-cheng commented 3 years ago

Changes:

jrtc27 commented 3 years ago

Why is this still introducing a new way of doing things, .option arch <foo>, rather than reusing and extending the existing syntax? This still results in two different ways to enable/disable RVC. We should not be adding new syntax unless absolutely necessary.

palmer-dabbelt commented 3 years ago

IMO this is the right way to go: having .option arch +c mean the same thing as .option rvc is fine, it's just a more general way of doing things. It's way cleaner than trying to have a million ad-hoc '.option noZba` for every flavor of extension we want to flip on/off. I'd go a bit farther and suggest a handful of additions:

jrtc27 commented 3 years ago

IMO this is the right way to go: having .option arch +c mean the same thing as .option rvc is fine, it's just a more general way of doing things. It's way cleaner than trying to have a million ad-hoc '.option noZba` for every flavor of extension we want to flip on/off. I'd go a bit farther and suggest a handful of additions:

Just as you wouldn't implement this by having a million ad-hoc -Zba options for .option arch, you wouldn't implement my proposal that way either, you'd parse (no)rv and then parse an extension name.

  • Add an = op (or just let it be blank). This would let users write something like .option arch rv64gc when they want to control exactly the arch they're based on.
  • Drop the extension list, and just make users insert multiple .option arch lines. This prevents us from getting screwed when our separator (IIUC a ,?) becomes a valid character in extensions.
  • Add this as command-line syntax. Something like -march=rv64g -march+c. We can deal with that later, though, as I'm sure it'll require a lot of sorting out.
  • Add this as a function attribute. So something like int func() __attribute__(("arch", "-c")) or whatever the syntax usually looks like over there. We can also argue about this one later.

That's __attribute__((target(...))) as an architecture-independent attribute (with architecture-dependent arguments), and __attribute__((target("-c"))) etc already work today for LLVM (in as much as it can without assembly syntax to toggle things on and off; for C in particular it won't work well, or at least not if you round-trip through assembly, might work fine if you don't, but for something like M or F it'll just (not) emit the instructions).

kito-cheng commented 3 years ago

IMO this is the right way to go: having .option arch +c mean the same thing as .option rvc is fine, it's just a more general way of doing things. It's way cleaner than trying to have a million ad-hoc '.option noZba` for every flavor of extension we want to flip on/off. I'd go a bit farther and suggest a handful of additions:

Just as you wouldn't implement this by having a million ad-hoc -Zba options for .option arch, you wouldn't implement my proposal that way either, you'd parse (no)rv and then parse an extension name.

To me, both are extending stuffs, and now we have chance to implement one scheme more clear / better, so I would prefer the new one. (although that's subjective I know.)

kito-cheng commented 3 years ago

Add an = op (or just let it be blank). This would let users write something like .option arch rv64gc when they want to control exactly the arch they're based on.

Sound good idea.

Drop the extension list, and just make users insert multiple .option arch lines. This prevents us from getting screwed when our separator (IIUC a ,?) becomes a valid character in extensions.

Hmmm, make sense to me...

Add this as command-line syntax. Something like -march=rv64g -march+c. We can deal with that later, though, as I'm sure it'll require a lot of sorting out.

Yeah, I guess we need that to deal with the RISC-V profile...although I prefer use -march as the only option for control the ISA.

Add this as a function attribute. So something like int func() attribute(("arch", "-c")) or whatever the syntax usually looks like over there. We can also argue about this one later.

Like @jrtc27 said, LLVM having some level of support for that, write down that into to https://github.com/riscv/riscv-c-api-doc is my next step, but for GNU toolchain site, we need this to implement that.

palmer-dabbelt commented 3 years ago

IMO this is the right way to go: having .option arch +c mean the same thing as .option rvc is fine, it's just a more general way of doing things. It's way cleaner than trying to have a million ad-hoc '.option noZba` for every flavor of extension we want to flip on/off. I'd go a bit farther and suggest a handful of additions:

Just as you wouldn't implement this by having a million ad-hoc -Zba options for .option arch, you wouldn't implement my proposal that way either, you'd parse (no)rv and then parse an extension name.

IMO that's pretty ad-hoc: by removing the explicit name-spacing provided by Kito's proposal we end up with these options all at the top level, which makes them harder to describe. That said, I don't really care that much about syntax.

  • Add an = op (or just let it be blank). This would let users write something like .option arch rv64gc when they want to control exactly the arch they're based on.
  • Drop the extension list, and just make users insert multiple .option arch lines. This prevents us from getting screwed when our separator (IIUC a ,?) becomes a valid character in extensions.
  • Add this as command-line syntax. Something like -march=rv64g -march+c. We can deal with that later, though, as I'm sure it'll require a lot of sorting out.
  • Add this as a function attribute. So something like int func() __attribute__(("arch", "-c")) or whatever the syntax usually looks like over there. We can also argue about this one later.

That's __attribute__((target(...))) as an architecture-independent attribute (with architecture-dependent arguments), and __attribute__((target("-c"))) etc already work today for LLVM (in as much as it can without assembly syntax to toggle things on and off; for C in particular it won't work well, or at least not if you round-trip through assembly, might work fine if you don't, but for something like M or F it'll just (not) emit the instructions).

Sounds like that should be in some spec somewhere, as I don't remember having seen it before. I also very much question the value of providing options that generate broken code.

jrtc27 commented 3 years ago

IMO this is the right way to go: having .option arch +c mean the same thing as .option rvc is fine, it's just a more general way of doing things. It's way cleaner than trying to have a million ad-hoc '.option noZba` for every flavor of extension we want to flip on/off. I'd go a bit farther and suggest a handful of additions:

Just as you wouldn't implement this by having a million ad-hoc -Zba options for .option arch, you wouldn't implement my proposal that way either, you'd parse (no)rv and then parse an extension name.

IMO that's pretty ad-hoc: by removing the explicit name-spacing provided by Kito's proposal we end up with these options all at the top level, which makes them harder to describe. That said, I don't really care that much about syntax.

  • Add an = op (or just let it be blank). This would let users write something like .option arch rv64gc when they want to control exactly the arch they're based on.
  • Drop the extension list, and just make users insert multiple .option arch lines. This prevents us from getting screwed when our separator (IIUC a ,?) becomes a valid character in extensions.
  • Add this as command-line syntax. Something like -march=rv64g -march+c. We can deal with that later, though, as I'm sure it'll require a lot of sorting out.
  • Add this as a function attribute. So something like int func() __attribute__(("arch", "-c")) or whatever the syntax usually looks like over there. We can also argue about this one later.

That's __attribute__((target(...))) as an architecture-independent attribute (with architecture-dependent arguments), and __attribute__((target("-c"))) etc already work today for LLVM (in as much as it can without assembly syntax to toggle things on and off; for C in particular it won't work well, or at least not if you round-trip through assembly, might work fine if you don't, but for something like M or F it'll just (not) emit the instructions).

Sounds like that should be in some spec somewhere, as I don't remember having seen it before. I also very much question the value of providing options that generate broken code.

https://clang.llvm.org/docs/AttributeReference.html#target https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html (scroll way down)

It works just fine for everything but C. For C it'll still mess with the subtarget internally (which can affect codegen for relative costs of instructions as it looks at whether they're compressible) but we fail to omit the necessary .option (no)rvc to stop post-isel compression, and it seems that does affect using the integrated assembler too (not sure how though, bit strange). There's nothing wrong with the option, just a bug in its implementation for one specific case (that's a rather weird one).

jrtc27 commented 3 years ago

IMO this is the right way to go: having .option arch +c mean the same thing as .option rvc is fine, it's just a more general way of doing things. It's way cleaner than trying to have a million ad-hoc '.option noZba` for every flavor of extension we want to flip on/off. I'd go a bit farther and suggest a handful of additions:

Just as you wouldn't implement this by having a million ad-hoc -Zba options for .option arch, you wouldn't implement my proposal that way either, you'd parse (no)rv and then parse an extension name.

IMO that's pretty ad-hoc: by removing the explicit name-spacing provided by Kito's proposal we end up with these options all at the top level, which makes them harder to describe. That said, I don't really care that much about syntax.

I agree that, if I could go back in time, I would stop the (no)rvc syntax from being adopted by GNU as and pick something more like this proposal. However, my personal opinion is that there should be a very good reason for introducing new syntax for something that can be done by generalising existing syntax, but .option arch is certainly better than .push_arch. I think the way forward is to bring it up with both GNU and LLVM developers to see if there's a consensus for which of the two current proposals they feel is best.

jrtc27 commented 3 years ago

NB: I would personally put a comma after .option arch, as is done for .attribute arch, if that is the proposal that ends up being adopted.

kito-cheng commented 3 years ago

Changes:

asb commented 3 years ago

I agree that .option arch is just a more general way to specify things, and I prefer keeping things namespaced there.

We discussed somewhat on the LLVM sync-up call and there were diverging opinions on whether it was desirable to later deprecate .option [no]rvc (either just documenting it's no longer the preferred way, or even moving towards emitting warnings). But I don't think that's a discussion that needs to be had here and now.

Nelson1225 commented 3 years ago

Hi Guys, Is there any further concern about this issue? If no, then I think maybe it is time to merge this pr, and then we could proceed to the next issues, including add mapping symbols with ISA string to these .option arch directives. Thanks.

jrtc27 commented 3 years ago

Hi Guys, Is there any further concern about this issue? If no, then I think maybe it is time to merge this pr, and then we could proceed to the next issues, including add mapping symbols with ISA string to these .option arch directives. Thanks.

Mapping symbols are not dependent on this; .option (no)rvc already exists and could make use of them, even if not really necessary. As could files compiled with different -march= strings.

kito-cheng commented 2 years ago

Changes:

kito-cheng commented 2 years ago

Changes:

luxufan commented 2 years ago

Hi, @kito-cheng . I have a question that since D extension will imply F extension, should '.option arch, -d' also disable F extension at the same time?

jrtc27 commented 2 years ago

Why wouldn't that just leave you with F? The only weird thing is that .option arch, =rv32i, +d, -d will give you rv32if, but I think that's the least bad semantics and hard to oddities like that.

kito-cheng commented 2 years ago

I have a question that since D extension will imply F extension, should '.option arch, -d' also disable F extension at the same time?

No, since that means we also need to disable zicsr due to F imply zicsr :( But thanks for point out that I think we should add NOTE to mention that.

The only weird thing is that .option arch, =rv32i, +d, -d will give you rv32if, but I think that's the least bad semantics and hard to oddities like that.

@jrtc27 I guess we should add a NOTE to recommend user use push/pop rather than -, consider =rv32i, +v, -v, will result rv32ifd_zve32x_zve32f_zve64x_zve64f_zve64d_zvl32b_zvl64b_zvl128b.

kito-cheng commented 2 years ago

Changes:

kito-cheng commented 2 years ago

Changes:

kito-cheng commented 2 years ago

@luismarques thanks!

kito-cheng commented 2 years ago

@luismarques @asb do you mind take a review for this? this has been verified on binutils for a while and here also a LLVM implementation already under review: https://reviews.llvm.org/D123515

kito-cheng commented 1 year ago

We've binutils support on upstream and a patch on LLVM (https://reviews.llvm.org/D123515), so I think it's time to merge this, before merge this, I would like wait one more week to make sure no objection or further comment from LLVM community @asb @luismarques @jrtc27 @MaskRay.

kito-cheng commented 1 year ago

Thanks everyone! We have the last necessary element of the function multi-versioning now!