In D65649#1943856, @simoncook wrote:

I agree, it seems more like a sketch of what they should be, but the mentioning of the size improvements suggested to me perhaps they should be, since evaluating the potential "what the final B extension should look like" it probably should be included. There is the question of where this should be enabled, my first thought (which this patch implements) is Zbb because it seemed the most logical, but maybe given the instructions stated state does another subextension make sense for its evaluation? These are all experimental so I think there's some leeway to some option, but I think it would be good to land it if we think people may be using it/want to evaluate it. I suspect there's many valid routes with this being experimental, we just need to choose one we have concensus around making sense. Perhaps a topic for today's call?

Submitting the comments I have so far - need to continue going through in more detail.

This is a much simpler fix! Looks good to me.

I'm reviewing this with an eye to merging it, but one big thing that comes to mind is the compressed instructions. The draft bitmanip spec describes these under "Future compressed instructions" and says "It presumably would make sense for a future revision of the “C” extension to include compressed opcodes for those instructions." My reading is that this is more of a sketch of potential encodings and a less firm proposal than the 32-bit encodings described elsewhere in the spec. Do you disagree with that assessment?

Thanks for the patch. I added a couple of nits - please rename the test file to thread-pointer.ll to match what other backends do and make it more discoverable. Also, although there different is minor we do use update_llc_test_checks.py for essentially all RISC-V tests so it would be good to use that for thread-pointer.ll as well.

I was thinking about making it clearer why this cast is necessary. The first thought was obviously to add a comment to explain (similar to the comment in your review description). But maybe there's an argument for adding a small helper? Although it would literally just be doing a cast, it's name and description should make it much clearer what is going on. I don't feel super strongly about that approach though - so do speak up if you disagree!

This LGTM, thanks Luis!

In D75168#1909097, @luismarques wrote:

In D75168#1907941, @asb wrote:

There are some cases where you need to perform more checking in order to be future proof for possible new standard extensions. Specifically, review the RVC Instruction Set Listings in the RISC-V spec and check for where an encoding is marked as "RES" (reserved). e.g. C.LWSP is valid only if rd!=0, and the version with rd=0 is a reserved encoding. Similar with C.LDSP. I think it's just those two.

I'm not sure that's something we want to do. Those are reserved for HINT instructions. The spec says:

This HINT encoding has been chosen so that simple implementations can ignore HINTs alto-
gether, and instead execute a HINT as a regular computational instruction that happens not to
mutate the architectural state.

If the core truly supports those HINTs then it shouldn't trap. If we are trapping presumably it's because the core is just executing the HINT as a plain load/store. That would typically be a NOP, but the address must be invalid and so we are trapping. In that case we are correctly identifying it as a READ or WRITE.

In D65649#1931064, @simoncook wrote:

This is starting to look good. I've checked all the encodings, and other than c.zext.w having the wrong value all the encodings are right.

One thing I would consider is to make reviewing/future changes easier, I would define all the instructions in the order they appear in the Chapter 2.11 Opcode Encodings tables. This way, it will be easier to see for future revisions what has changed and make sure the encodings are up to date. This might end up with a few more let Predicates = directives, but we can rearrange and tidy this up when it is all ratified.

LGTM, thanks Simon!

There are some cases where you need to perform more checking in order to be future proof for possible new standard extensions. Specifically, review the RVC Instruction Set Listings in the RISC-V spec and check for where an encoding is marked as "RES" (reserved). e.g. C.LWSP is valid only if rd!=0, and the version with rd=0 is a reserved encoding. Similar with C.LDSP. I think it's just those two.

LGTM, thanks!

Hi Dylan - on the mailing list thread for this I raised a concern about disassembler support based on comments I'd seen in a recent patch. Can you please clarify the current status there?

Yes, if @jrtc27 commits I'd be supportive of a cherry-pick to the release branch.

In D68964#1884580, @davidlt wrote:

Yes, please backport (if possible) all other related changes mentioned above (if they are not yet part of 10.0).

This would help distributions to avoid hacking LDFLAGS / build systems files while building LLVM/Clang stack on Linux.

@hans this patch fixes a reported bug https://bugs.llvm.org/show_bug.cgi?id=42984 and is non-invasive. I'd be in favour of cherry-picking for 10.0 if you're ok with that.

Great fix, thanks @jrtc27. Please do go ahead and commit.

In D68964#1882377, @hans wrote:

In D68964#1880966, @smeenai wrote:

CC @hans for 10.0

+asb from code_owners.txt, what do you think?

Thanks for implementing this - I'd personally like to see the psABI proposal https://github.com/riscv/riscv-elf-psabi-doc/pull/131 merged before merging into LLVM/LLD.

@MaskRay, @ruiu, would you be in favour of this being backported to 10.0?

I've filed a request to merge this into the release branch here https://bugs.llvm.org/show_bug.cgi?id=44631 - thanks for the fix James!

This looks good to me, thanks James. I had a closer step through of the logic here to convince myself.

Thanks for the fix! As Sam says, explaining the issue in the commit message would be useful.

In D67495#1811715, @lenary wrote:

In D67495#1800935, @luismarques wrote:

LGTM. My only concern was if it made sense to use the same statistic to count in both places, and if we could end up double counting the instructions emitted (now, or in a future LLVM version). After a quick look I didn't really see other targets using the same approach, but I also can't think of a way where this ends up actually being problematic.

My understanding is that instructions only go through one of these functions. RISCVAsmParser.cpp is used by the assembler only, and the RISCVAsmPrinter.cpp is only used by LLVM CodeGen. This should mean that instructions are not double-counted (today). Yes I'm not sure why we have both, I think @asb did this to ensure better layering.

LGTM, thanks.

Please update the check so it checks the error location too, like we do for other *invalid.s checks.

LGTM, thanks!

I note that the TargetFrameLowering hooks canUseAsPrologue and canUseAsEpilogue are both called by the shrink wrapper. They default to true, but targets may need to override this for correctness. Looking at e.g. AArch64, I see it overrides canUseAsPrologue and returns false in the case that the function needs stack realignment and there are no scratch registers available. Are you certain that no such case is needed for RISC-V?

LGTM, thanks Luis.

Thanks Simon, LGTM. I did wonder about just saying e.g. "the 'c' instruction set extension", but on balance I can't see it being any easier to understand or more obvious what to change, and it's handy giving a longer description.

Compared this to commits that introduced assembly mnemonic spellchecking for other backends, and it looks good to me. Thanks Simon!

Please update the commit message to clarify the cases where we do deviate from the GCC defaults, but this looks good to me.

Thanks James - won't this still leave problems for structs that need flattening?

LGTM, thanks!

In D67185#1707849, @lenary wrote:

Note, D68862 is in-progress at the moment, which is related to this patch.

LGTM, thanks!

Thanks Ed, could you add some tests to test/MC/RISCV/rvf-aliases-valid.s please?

I think this patch is orthogonal to save/restore via libcalls, and we can probably land it sooner. Could you please rebase so it's standalone?

@schwab - do you need someone to commit this for you?

I like the direction of this, Thanks Ana. Did you look at modifying getInstSizeInBytes? That would benefit other callers beyond the outliner.

Plus our dev meeting coding lab last year stepped through a similar optimisation, so we really should land it.

This LGTM, thanks. Added a few nits. I think Sam's suggestion of adding a helper function or two to handle the register arithmetic is worth evaluating, but doesn't need to be done in this patch.

In D67423#1671595, @jrtc27 wrote:

I'm still personally not fond of the Fx_F/Fx_D convention. The fact that F0_F < F1_F < F0_D < F1_D is highly surprising, especially when F0_32 < F0_64 < F1_32 < F1_64, and not what one would naively assume. I also don't know whether this peculiarity of TableGen is meant to be something that's relied upon. I know it doesn't match the ISA convention of always calling them Fx, but Fx and Dx have precedence in other backends and do not risk this same confusion. Moreover, the spec itself is highly inconsistent in how to refer to single and double precision floating point:

LGTM, thanks!

Thanks for pushing this forwards Sam. For the RISC-V backend we've been using a downstream shell script for this for ages, and I've always felt it would be better if the torture suite could be used from the LLVM test suite. It's been a while since I went over the excluded tests, it's possible it would benefit from further review if all the masked tests really should be masked.

LGTM, thanks!

This LGTM, but given how much discussion there has been about MaxPromoteWidth it would be great to get some test coverage for it.

This triggers when building the Linux kernel too. I forgot to submit this patch after getting that working. I also couldn't get a meaningful test case for it that didn't seem like it would be horribly brittle. LGTM, thanks.

Resigning as a reviewer, as the intent of this change is captured in rL366327.

Looks good to me, thanks! We may later review whether the ability to enable/disable these hints is worth it or not (I know I'd been in favour from the start, but looking at it with fresh eyes I'm not sure it's fully necessary), but this is a useful step forwards.

Actually changing my mind. This would be better tested by adopting something like arch-reg-names.s from D66139 (and extending that to handle FP regs), and leaving the inline-asm tests alone (huge increase in test size, which makes them harder to follow). Make that change, and then it looks good to me.

LGTM, but wouldn't it be worth moving arch-reg-names.s to D65950, and also expanding it to handle FP regs?

LGTM, thanks!

This causes failures on the GCC torture suite:

Also, clang-format seems to prefer to format some of this patch differently - worth running it through again.

Please to submit a follow-on patch for targetHandlesStackFrameRounding

In D65634#1618443, @rogfer01 wrote:

Thanks @asb @lenary for the review!

I understand that, after this change, we will also want to make -march=rv{32,64}gc the default in Linux as well. Otherwise there will be an ABI mismatch with the default -march=rv{32.64}i in a default invocation.

Does this make sense?

Many thanks for the patch. Could you please add some tests for this behaviour? I imagine you'll want to add a new directory in test/Driver/Inputs with a Debian tree skeleton. See D63497 for a relevant example.

In D62007#1614808, @lenary wrote:

Address review feedback

• implement targetHandlesStackFrameRounding to prevent stack being rounded twice.

Thank Roger. While we're doing this, I think it would make sense to default to ilp32d on 32-bit Linux? I know the glibc support etc is less mature than for RV64, but it seems the sensible thing to do.

Adding explicit LGTM in the Phabrictor UI. Thanks again Lewis!

Thanks for looking at an alternate approach here James. In light of the better error messages I'm going to go with D65205 at this point, but I'm open to future refactoring or improvements. I haven't spent a lot of time considering it, but my initial reaction on PushParens is I feel it might make the parsing logic harder to follow in return for a fairly minor reduction in repetition. But I could be convinced otherwise...

Looks good to me - thanks!

Added a nit around a doc comment, but this is looking good - thanks Sam. I agree that the error messages are nicer in this patch vs D65291 for now. However I'm very open to further improvements and refactoring of AsmParser logic in the future in follow-on patches. Only caveat to that is that the asm parser for any backend isn't particularly pretty, and that sometimes a little bit of repetition can be easier to read than an all-singing-all-dancing function that can do anything based on what function args you pass.

LGTM, thanks!

Getting sufficient LGTMs is a bit of a judgement call with LLVM. If the change is large and invasive, then typically you'd wait for the OK from the relevant code owner. For something like this, totally find for anyone active in that part of the codebase to approve.

This looks good to me, thanks! Upon review I considered whether evaluateConstantImm should always set VK, but it seems sensible to rely on the caller to initialise it if necessary, as setting VK when evaluateConstantImm failed seems surprising.

Looking at this in more detail - shouldn't we be making use of the R_RISCV_SET* relocations?