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?

LGTM, thanks. I've spent quite a bit of time comparing output vs gcc so I'm pretty confident here now. I committed a slightly different test strategy in the DWARF encoding work I just committed, so will adjust this patch to build on that.

In D63409#1587351, @edward-jones wrote:

In D63409#1587117, @asb wrote:

Hi Ed, this is really hard to review without any documentation in the psABI or otherwise. Can you confirm that you've verified this matches gcc/gas? Are you planning to submit document to riscv-elf-psabi-doc to reflect this?

I've opened a pull request on the psABI docs (https://github.com/riscv/riscv-elf-psabi-doc/pull/110). I'm not entirely sure the details here are relevant to the ABI though. As far as I'm aware provided the individual CIE and FDEs are well-formed it shouldn't matter if the encodings differ from those specified in other CIE and FDEs.

On another note, after taking a look at the GCC code for this I'm not convinced the non-PIC values for the encodings match. GCC appears to just encode everything as pc-relative sdata4, whereas the above encodes things as absolute when PIC is not specified.

Hi Ed, this is really hard to review without any documentation in the psABI or otherwise. Can you confirm that you've verified this matches gcc/gas? Are you planning to submit document to riscv-elf-psabi-doc to reflect this?

Thanks for the clarification Ed. This looks good to me - thanks!

In D55277#1586965, @plotfi wrote:

In D55277#1586961, @asb wrote:

In D55277#1586959, @plotfi wrote:

In D55277#1586958, @asb wrote:

In D55277#1586957, @plotfi wrote:

Test failures in lld tests: http://lab.llvm.org:8011/builders/lld-x86_64-ubuntu-fast/builds/3357

Sorry about that, patch incoming.

I got one coming too. Should I wait for yours or just land this one?

Oh great, please land yours then. Mind is written blind as lld doesn't seem to be building after I add it to LLVM_ENABLE_PROJECTS. Will have to wipe my build dir and wait a while.

Would you be able to do a post commit look over: https://github.com/llvm/llvm-project/commit/3e10905c49ffda36d7a90e24be1ab30832fa4afc

??

Thanks

PL

In D55277#1586959, @plotfi wrote:

In D55277#1586958, @asb wrote:

In D55277#1586957, @plotfi wrote:

Test failures in lld tests: http://lab.llvm.org:8011/builders/lld-x86_64-ubuntu-fast/builds/3357

Sorry about that, patch incoming.

I got one coming too. Should I wait for yours or just land this one?

In D55277#1586957, @plotfi wrote:

Test failures in lld tests: http://lab.llvm.org:8011/builders/lld-x86_64-ubuntu-fast/builds/3357

LGTM, thanks! Some minor rebasing is needed for relax-debug-frame.ll. Please go ahead and commit.

LGTM (two minor comments in-line). Pleas go ahead and commit - thanks!

Thanks Ed. I came to the same conclusion re the need for emitting R_RISCV_32_PCREL when debuggin some issues over the weekend. I had a different fix though - I'll try to compare vs this and get back to you later today.

LGTM, thanks!