This patch implements 1.0-RC3:
https://github.com/riscv/riscv-aia/releases/download/1.0-RC3/riscv-interrupts-1.0-RC3.pdf
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
This extension doesn't appear to be ratified but you've listed it in the table of ratified extensions and treated it as a ratified extension in RISCVISAInfo.cpp. I know that given we don't do checking for CSR names the distinction feels a bit academic, but I don't think this is the best approach. For CSR only extensions like this my feeling is we should review a patch like this ahead of ratification, but not land until it's actually ratified. One of the main goals (from my perspective at least) for allowing not-yet-ratified extensions upstream was to allow collaboration on the implementation, but given the simplicity of CSR-only extensions I'm not sure the cost/benefit makes sense vs just waiting for ratification when we can be sure the assigned numbers won't change. I'd be open to counter arguments though.
Comment Actions
I would be fine landing this as experimental before ratification. I see no real downside to doing that, and would essentially leave it the judgement of the patch author as to whether just waiting until ratification or doing the extra work for staging as experimental is worthwhile.
Comment Actions
My concern would be that as we don't gate CSR names on enabling the relevant extension, people could start using CSR names and encodings that could change, without opting in via -menable-experimental-extensions, perhaps not realising that they're using the unratified version. OTOH, you could argue it was user error from the start by not trying to specify all the needed extensions in the ISA naming string.
Comment Actions
My concern would be that as we don't gate CSR names on enabling the relevant extension, people could start using CSR names and encodings that could change, without opting in via -menable-experimental-extensions, perhaps not realising that they're using the unratified version. OTOH, you could argue it was user error from the start by not trying to specify all the needed extensions in the ISA naming string.
We decide don't gate CSR before, but I am wondering maybe we should gate those CSR if they are defined by a unratified/experimental ext., and remove the checking once it ratified, since it might change the name or CSR number before ratified.
Comment Actions
That's definitely a risk, but at least when people try to do the "right" thing and specify the extension name in the ISA string, they'll quickly find out that the version supported in the compiler is experimental. Given the cost of marking such CSR extensions as experimental is near-zero, I think we might as well. I agree gating the CSR names might also make sense, but then we're back to the effort of testing this. I think our conclusions before were that nobody is particularly opposed to doing finer grained control of CSRs for new extensions at least (it's more problematic for the older CSRs given spec changes to move CSRs to separate extensions), so I think it would be find if someone wanted to implement that. I don't think we need that as a pre-requisite for this patch though, IMHO.
Comment Actions
I think I agree with Kito here. Requiring gating for experimental extensions even if we don't gate ratified ones seems like a good way to address the issue of name collision risk in experimental extensions.
I'm less concerned about number reassignment. I'm most concerned that the same name might be reused across two extensions in a mutually incompatible way. This would clearly get fixed before both were ratified, but well, the entire point of experimental is that they not yet ratified.
Comment Actions
Just noting for posterity that we discussed this at last week's RISC-V sync-up call and I think the tentative conclusion (there weren't particularly strong views) was that experiments CSRs should really be gated by -menable-experimental-extensions, with the CSR names gated so as to avoid the issues mentioned here.