- User Since
- Aug 6 2013, 5:31 AM (503 w, 2 d)
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?
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.
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?
Thanks Jessica - in that case, LGTM once it has the MIR test case Jessica requested.
Tue, Mar 28
Mon, Mar 27
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 - 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.
Sun, Mar 26
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.
Fri, Mar 24
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.
Thu, Mar 23
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.
Tue, Mar 21
In principle I think this is good, but I see two considerations:
- 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).
- 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.
Mon, Mar 20
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.
Fri, Mar 17
This broke -DBUILD_SHARED_LIBS=True builds - I committed rG482d22d05a4a30a4f8594273bd359f7d311c9d4c to fix it.
Thu, Mar 16
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.
Wed, Mar 15
LGTM - thanks!
Thanks for making this suggestion.
Tue, Mar 14
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.