LGTM, though hopefully at least one other person can confirm they're happy with the wording.

LGTM. I also understand that Rocket and SCR-1 support zicsr and zifencei in all standard configurations (and their respective repos seem to confirm this).

LGTM, thanks (though see minor note about tweaking commit message to clarify the change).

I've added a couple of inline comments, but otherwise this seems fine to me. I'd suggest updating the patch description to reference Philip's documentation patch (which was posted soon after this), and also to explain why there are no codegen changes (I think "Either changes to the specification required no codegen changes, or LLVM was written against a newer version of the specification than it claimed" is vague but accurate).

I think this reflects what we (extensively) discussed and there was broad consensus for, so I'd be happy enough for it to land as-is rather than getting too bogged down over precise terminology. That said, I do think that going into the details of draft vs non-draft specifications and so on might be going into more detail than necessary, and could be confusing. It also seems like RISC-V is rather ignoring (and perhaps choosing to move away from?) the task of centralising all the extensions in a single ISA manual, so again it may make more sense to focus on ratified vs non-ratified as the thing that is easily understandable rather than trying to explain the way extension definitions are confusingly spread across different repositories.

@kito-cheng is this something you might be able to help review?

In D147173#4232034, @craig.topper wrote:

In D147173#4231929, @asb wrote:

LGTM. I'd note that clang-format prefers different formatting for some of the functions here - it's probably worth reformatting while you're doing this merge into a new pass.

Is it just the switch statement I intentionally wrote with the lines compacted?

LGTM.

LGTM. I'd note that clang-format prefers different formatting for some of the functions here - it's probably worth reformatting while you're doing this merge into a new pass.

LGTM.

Brilliant, thanks everyone. @vit9696 am I right that you don't currently have LLVM commit access - would you like me to go ahead and commit this for you?

In D146191#4230950, @craig.topper wrote:

In D146191#4230864, @asb wrote:

Thanks Jessica - in that case, LGTM once it has the MIR test case Jessica requested.

I thought the earlier conclusion was that the RISC-V part wasn’t needed if T-Head moves their pseudo expansion in their downstream.

Thanks Jessica - in that case, LGTM once it has the MIR test case Jessica requested.

In D143708#4212196, @MaskRay wrote:

No objection. LG once @jrtc27 is happy.

LGTM too.

In D146191#4226495, @pcwang-thead wrote:

In D146191#4225567, @craig.topper wrote:

Do the T-Head specific pseudos being expanded need to expanded as late as the other pseudos? Could you have an earlier expansion pass? I was already nervous about the short forward branch instructions being affected before. I know we determined those cases were ok.

Yes and thanks, I just tried. They can be placed in addPreEmitPass() just like MCP.
Apart from this, do you think it's still worthy marking outlined functions?

In D135960#4209064, @asb wrote:

In D135960#4177411, @asb wrote:

Could you please update the patch description to say "Fixes #57461" or similar?

I think all the review comments are addressed and I don't have additional feedback - the only open line of discussion I think was about how fixup_riscv_12_i is the only fixup that can never result in a relocation (as pointed out by @jrtc27). That's true, but I also can't see an alternative way of handling it or any real disadvantage of this - it logically _is_ a fixup. Do you see an alternative approach, or was it more that you think it would make sense to adjust the naming so it's clearer it doesn't have a matching relocation?

Rereading this, I wasn't very clear who I was directing my question to - I meant to direct it at @jrtc27 - what do you think?

LGTM

I can't say I've done any real performance analysis, but this seems sensible to me and I didn't spot any obvious regressions from kicking the tires.

I've flagged this proposed change on Discourse (more the fact we're looking to change it, rather than the precise register we end up with).

LGTM.

LGTM - I just wonder about if we want to gate this on C/Zca as this patch does. Assuming someone reused the 16-bit encoding space, perhaps they want to using .insn to insert some of their instructions. OTOH, perhaps it's not so likely that the instruction formats would be as they want, at which point .insn isn't very useful.

Not as compelling as user facing strings or comments that clearly refer to the RISC-V ISA rather than the "RISCV" LLVM backend. But I don't really see a good reason not to be consistent. LGTM.

LGTM.

Rebase and address review comments.

Rebase (to account for removal of string from RISCVExtensionInfo. The following patches have been approved, so just waiting for approval of this to land the series.

I start writing a comment grumbling that this adds undesirably tight coupling between with the implementation details of TargetLowering::shouldFormOverflowOp, but when I got to the end of it figured it wasn't that bad. If the comment had just said "Allow the transform whenever it would be allowed for UADDO" I might not have commented. LGTM. Though an assert like you suggest probably makes sense.

First and foremost, thank you for keeping this up to date and iterating upon it while the .option arch proposal was under review in the asm manual. I think we're basically almost there.

I don't feel comfortable approving this because I played a hand in diagnosing the issue and helping Mikhail with the patch. I do think updating our handling of the 'm' constraint to match GCC's is the right thing to do, and this patch achieves it in the simplest way I'm aware of. The logic around asm operand constraints isn't always easy to follow, so it's possible I'm missing alternative ways of achieving the same goal.

Apologies, the commited version of this patch missed the clang/test/Driver/riscv-arch.c change. rGe54cdd058e223bd62840e901b8b462c011d2fae5 committed to fix this.

LGTM - thanks. I'll land this for you now since it already got a LGTM from Kito.

I expect you'll need to / want to update the C++ RISCVISAInfo unit tests as they were added after this patch was first proposed.

In principle I think this is good, but I see two considerations:

1. Sam was right that there's an ability to change the register after the fact, but 2.5 years have passed since then. We should make a good faith attempt to see if any downstream users are relying on the current ABI (e.g. the original contributors - @zzheng and @apazos).
2. Now that the issue has been raised in the psABI / toolchain-sig, it would be good if there were consensus on the "platform register" rather than changing this multiple times if some different register is decided.

In D135960#4177411, @asb wrote:

Could you please update the patch description to say "Fixes #57461" or similar?

I think all the review comments are addressed and I don't have additional feedback - the only open line of discussion I think was about how fixup_riscv_12_i is the only fixup that can never result in a relocation (as pointed out by @jrtc27). That's true, but I also can't see an alternative way of handling it or any real disadvantage of this - it logically _is_ a fixup. Do you see an alternative approach, or was it more that you think it would make sense to adjust the naming so it's clearer it doesn't have a matching relocation?

The logic I saw with the pass names is that the backend for the RISC-V architecture is called "RISCV". But I've got no objection to changing.

LGTM.

This broke -DBUILD_SHARED_LIBS=True builds - I committed rG482d22d05a4a30a4f8594273bd359f7d311c9d4c to fix it.

In D143248#4128698, @varunkumare99 wrote:

In D143248#4122933, @asb wrote:

Hi Varun - thanks for filing bug #60698 to help track this issue, and for noticing that there should now be some upstream support for cfi_remember_state and cfi_restore_state.

The first question that comes to my mind is what degree of testing you've done with this?

Hi Alex,
the patch updates over 500 testcases for RISCV(as it adds cfi info the epilogue), which results in a patch of over 30M. Due to which I'm not able to upload.

I haven't looked through the .td description yet, but left some first comments (all pretty easily addressable and minor).

LGTM. I think I can see the potential reasoning for the extension name being in RISCVExtensionInfo in the first place - it makes it more plausible to use it separated from the OrderedExtensionMap. But as you note, in reality we don't have any such uses and I can't really foresee any either.

Rebase.

LGTM - thanks!

Thanks for making this suggestion.

LGTM. Could you precommit the tests though, as it's a bit confusing to add new tests when I think these pass even without the change.

LGTM.