Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I think this makes us need to consider another issue: Should we support I extension with version 2.1? zifencei and zicsr split from I ext. since 2.1.
RISC-V GNU toolchain site are consider to bump the extension version to newer version in next release, which means will default with I 2.1, A 2.1, F 2.2 and D 2.2.
Here's a proposed path forwards:
- Patch 1: Add support for RVI 2.1 and related changes. Keep RVI 2.0 as the default.
- Patch 2: Add warnings for uses of the removed fence.i, CSRs and counters when using RVI 2.0 (at least when doing so by default; arguably this should error out instead when explicitly specifying version 2.0.)
- Patch 3: Start defaulting to RVI 2.1. IMO this should only be committed after patch 2 being in effect for a while.
I tried to make I-ext imply zifencei, zicsr, and zihintpause automatically. BTW zihintpause is implemented in D93019.
It's a fairly minor detail, but I think implying zihintpause automatically is semantically different to what Luis proposed (as zihintpause wasn't in RVI 2.0, it shouldn't be implied by default like zifencei and zicsr should).
Luis' proposed series of patches makes a lot of sense to me, but a complicating factor is I'm not sure we have a super clean way to support different versions of an extension at the same time.
For instance, to make this patch match Luis' suggested "patch 1" you'd need to be able to specify rvi2.1 and then have it not automatically included zifencei and zicsr. Ideally the version emitted in build attributes would stay as 2.0 unless you explicitly selected 2.1 as well. This patch would then also not need to update test cases to include +zifencei and +zicsr.
We should discuss this in the RISC-V LLVM call today.
There is several issue around the default extension version stuffs.
- Should we add -misa-spec= option to Clang/LLVM?
- Behavior for zifencei and zicsr with i 2.0?
- How to encode the extension version in LLVM? by attribute or module flags?
Should we add -misa-spec= option to Clang/LLVM?
Here is a long discussion[1] at 2019, at that moment I think we all agree -misa-spec is a good solution,
However it's kind of awkward for this scheme is ISA spec changing the version scheme again after 20191213 release,
it's become NO formal release for long time, just bunch of draft release.
That's means NO -misa-spec version can cover vector, crypto and bit manipulation.
So we might consider different scheme on controlling default version,
or change the semantic of -misa-spec to make it no longer just reference version of RISC-V ISA manual,
maybe we could reference RISC-V profile[2], but the spec still under discussion.
For GNU toolchain land, -misa-spec= already implemented in GNU toolchain about ~ 2yrs,
so I prefer change the semantic to reference version other than version of RISC-V ISA manual
rather than adding new command line option to control that.
[1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/aZhMG7NIVTk/m/PTZEaTWiAwAJ
[2] https://github.com/riscv/riscv-profiles
Behavior for zifencei and zicsr with i 2.0?
I would suggest still accept i 2.0 with zifencei and zicsr for compatibility,
but NO effect and NO ELF attribute for those two extensions if I is 2.0,
however I don't have strong opinion on this issue.
How to encode the extension version in LLVM? by attribute or module flags?
It's kind of LLVM specific implementation issue, RISC-V LLVM use target attribute to
enable/disable specific extension, but we didn't encode version info in target attribute or module flags yet,
there is not too much problem on clang side during parsing arch string, but that cause some problem on ELF
attribute emission, +a means 2.0 or 2.1? +f means 2.0, 2.1 or 2.2? and what the version of i?
i has major change between 2.0 and 2.1, so we might consider adding new target attribute (e.g +i2p1?) to identify the version of i.
But f and d is clarification on the NaN behavior, which is related minor for toolchain, so adding new target attribute to that is kind of ... weird?
f/d 2.0
FMIN.S and FMAX.S write, respectively, the smaller or larger of rs1 and rs2 to rd.
f/d 2.2
Floating-point minimum-number and maximum-number instructions FMIN.S and FMAX.S write, respectively, the smaller or larger of rs1 and rs2 to rd. For the purposes of these instructions only, the value −0.0 is considered to be less than the value +0.0. If both inputs are NaNs, the result is the canonical NaN. If only one operand is a NaN, the result is the non-NaN operand. Signaling NaN inputs set the invalid operation exception flag, even when the result is not NaN.
Anyway, I think here is two possible solution, but I didn't have strong opinion here:
- Add new target attribute for each version.
- Encode whole arch string with explicit version into module flag.
Note: Ext. versions in RISC-V ISA spec 20191213: I 2.1, M 2.0, A 2.1, F 2.2, D 2.2, C 2.0, E 1.9, Zicsr 2.0, Zifencei 2.1
Outside of the I/F/D special cases, where F/D don't really matter and I2p0 is just I2p1Zicsr2p0_Zifencei2p0,, I thought the new policy was that ratified extensions would never be changed, only new extensions published, and thus version numbers are basically irrelevant other than to distinguish ratified from pre-ratified?
Thanks for writing all of that up Kito.
I just wanted to check my understanding on this point. It's certainly true that there hasn't been a new ISA manual release for a long time. Are you saying that there won't be a new ISA manual release (using whatever naming scheme) that incorporates the ratified versions of bitmanip, crypto, vector etc?
Outside of the I/F/D special cases, where F/D don't really matter and I2p0 is just I2p1Zicsr2p0_Zifencei2p0,, I thought the new policy was that ratified extensions would never be changed, only new extensions published, and thus version numbers are basically irrelevant other than to distinguish ratified from pre-ratified?
The I2p0 and I2p1 is the most problem now, and new policy seems intend to add new ratified ext. rather than extend or change, but I guess I just be more conservative here, since the I isn't expect to be changed before it change.
And we are trying to support *real* multi-version on clang/LLVM (e.g. try to support v 0.10 and 1.0 and emit right version on ELF attr.), so we are headache on this issue for a while, but this might not issue to upstream LLVM I guess.
Are you saying that there won't be a new ISA manual release (using whatever naming scheme) that incorporates the ratified versions of bitmanip, crypto, vector etc?
Oh, I guess I using some word too strong there, here should have some newer release in future, but we don't know the time-frame and release version scheme, maybe I should checked that with other foundation folks, and back to post the updated info here.
That already become a practical problem to GNU toolchain side since upstream GNU toolchain support those extensions who already frozen, and could enable that without version and -meanble-experimental-extension, but for Clang/LLVM side, it's still guarded by -meanble-experimental-extension and require explicitly version, so it's not urgent issue for Clang/LLVM
https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/aE1ZeHHCYf4
RISC-V GNU toolchain are going to bump the default ISA spec to 20191213, which means will default with I 2.1, A 2.1, F 2.2 and D 2.2.
I think it will be good if LLVM could supports I 2.1 and have the same default ISA version with gnu...
@achieveartificialintelligence Do you know what is the relation between your patch and D115921?
| clang/lib/Basic/Targets/RISCV.cpp | ||
|---|---|---|
| 236 ↗ | (On Diff #394174) | auto ParseResult = llvm::RISCVISAInfo::parseFeatures(XLen, /*IsI2p1=*/ false, Features); |
| clang/test/Driver/riscv-arch.c | ||
| 3 ↗ | (On Diff #394174) | could we have a check for -march=rv32i2p1? |
@achieveartificialintelligence Do you know what is the relation between your patch and D115921?
This commit doesn't depend on D115921, but if D115921 is merged, it's a good idea to build on it.