In D53235#1356067, @rogfer01 wrote:

Ah, I think what happens is that we should not emit such patterns without the presence of the F extension: these patterns are ultimately selected as instructions in F. Does this make sense?

Patch refresh, no changes. Ping?

Rebased (no meaningful changes).

Updated. Now handles cmpxchg, supporting all RV64A. The code changes are actually pretty minimal so I hope this is fairly straight-forward to review.

Thanks for the review @efriedma. Updated to add the suggested regression test.

Many thanks to @efriedma for catching the issue with one of the sdiv patterns. I've removed that pattern, and added similar logic to that used for udiv/urem so that anyext is converted to sext if this is likely to result in one of the *w instructions being selected (and thus avoiding unnecessary sext/zext of the input operands).

In D47755#1334053, @shiva0217 wrote:

Hi Alex, I have added a constant pool test case at the end of align.s to verify the padding behavior for constant pool. Once D45961 landing and changing the behavior, we should be able to aware. It seems that GCC will insert Nops for padding constant pool. Could you illustrate more about "use .align with a value to fill with"? I could test on GCC.

I think we've landed enough pre-requisites that this this is good to go in, but it introduces failures to option-pushpop.s and option-relax.s. Can you take a look please?

I know this isn't a behaviour change vs the previous bare_symbol, but I note that GNU as currently accepts call with constants, e.g. call 1234. Should we be doing the same?

Updated to address review comments from Eli (thanks!).

Previous patch was the pre-clang-format version. This update fixes that!

Updated to improve pattern definitions. The first patterns were over-constrained, as they would only match shifts where the shift amount operand matched the zexti32 pattern. I introduce the shiftwamt PatFrags so it will match a zexti32 shift amount (and hence avoid generating the unnecessary zero-extend), or a shift amount that isn't zero-extended.

Refreshed patch.

Refreshing patch, no functional changes.

Rebased on top of D56264 and D53230. Although this patch could be written to apply without those changes, given all patches modify the dag combines it's simpler to keep them linear.

Closed by rL350321 (sorry, forgot to mention this review in the commit message).

Rebasing patch so it is no longer dependent on D52298

As suggested my Shiva, this patch has been updated to allow AsmParser::parseExpression to do the work when parsing an identifier.

Updated the patch to address the correctness issues with the udiv and urem patterns. Like with D56264, a dag combine converts an ANY_EXTEND to a SIGN_EXTEND when it operates on an instruction where a 32-bit *W variant could be selected. This is advantageous as it can avoid unnecessary masking/sign-extension of the input operands.

Thanks, this basically looks good to me. Could you please add some tests for la with invalid operands.

This no longer seems to apply cleanly (needs rebase), but otherwise LGTM. Thanks!

In D54029#1329843, @jrtc27 wrote:

I personally would regard that as an implementation detail. There's no reason we couldn't give an error; we bail out of shouldForceRelocation early without bothering to check the relocation if relaxation is enabled, but there's nothing stopping us from putting the %pcrel_lo error check before that other than being slightly slower and increasing code complexity.

Marking as "planned changes". At least the following patterns are problematic:

This looks good to me, but there's one addition test I'd like to see. You explain that the intended behaviour is that there will be no compile-time error if linker relaxation is enabled. It would be good to have this reflected in a test. Maybe adding extra RUN lines to pcrel-lo12-invalid.s and adding a comment that explains that is expected with linker relaxation enabled.

Forgot to change the status in Phabricator prior to commit, but I did re-review the updated change and it definitely LGTM!

Thanks for sharing this patch series Lewis. As it happens @luismarques has been finishing off his own implementation of TLS support which this unfortunately clashes with. The upside is that we're in a good position to review this work!

@rogfer01: James did actually get a patch for this committed to riscv-asm-manual https://github.com/riscv/riscv-asm-manual/blob/master/riscv-asm.md

Thanks, fixed in rL348117.

I'll update this to include cmpxchg support now D48131 landed.

Just some minor tweaks and I think this is good to go. In addition to the changes mentioned in inline comments, please:

• add nounwind to all the test functions
• Remove tail from the calls to llvm.fma as it doesn't impact the generated code (I have a preference to remove everything that doesn't impact the generated code).

Looks good to me. I double-checked that gas doesn't do this same conversion for fsflags and fsrm. It doesn't, so this seems complete.

Thanks to Luís, there is agreement on c.unimp on the sw-dev mailing list, it's now documented in the RISC-V asm manual, and binutils now supports c.unimp too (courtesy of Jim Wilson).

LGTM. Needs a trivial tweak to apply. This can land now the RV64I patch was committed. I'll commit now (I believe Lewis doesn't have commit access).

Updated to add note about FPOWI being undefined if integer operand is > 32-bits, as suggested by @efriedma

Submitting some old in-line comments that were accidentally left unsubmitted.

I should probably plan to reflect support for this in my in-flight cmpxchg patch (which adds target-independent support for late lowering of cmpxchg in the same way I added it for atomicrmw). [incidentally - reviews on that patch would be very welcome...]

This patch still applies with minimal fuzz and all tests still pass.

• I'd like to find a solution that doesn't get rid of the helpful error reporting we currently have for load/store. Downgrading all errors to just "invalid operand for instruction" is an unfortunate regression
• The F/D load/store are wrong as this patch has them accepting GPRs rather than FPR32/FPR64. The outs for PseudoSTORE also has a mistake
• We no longer have the shouldForceImmediate logic because associating custom parsers with operands was found to be superior. From some playing with a modified version of this patch it looks like more logic is needed to get these aliases to parse though...

Looks good to me.

I think this is fine, but c.unimp should be proposed on the RISC-V sw-dev mailing list or similar. As we discussed, it seems to make total sense to have this instruction (every other compressed instruction is nameable with c.something). But we should get community feedback first before merging in to LLVM. It's probably also worth highlighting the fact that "unimp" isn't actually guaranteed to trap according to the current RISC-V specs...