Thanks for the patch Lewis. Could I please request the following changes:

• It would be handy to link to https://gcc.gnu.org/onlinedocs/gccint/Machine-Constraints.html in the patch summary, so people can easily double-check the constraint definitions for themselves
• Could you add tests for values outside of the expected range. AArch64 and X86 do this in test/Sema/inline-asm-validate-{aarch64,x86}.c
• It would be worth expanding riscv-inline-asm.c to provide simple sanity checks for "m" and "r"

This is an NFC patch and looks good to me.

Looks good to me. This won't cause any codegen issues because C_EBREAK is never gets selected in ISel anyway (EBREAK will get converted after it becomes an MCInst). But it's better to have the right metadata.

Refreshing patch, no changes.

Update to fix a typo in a comment and reflect changes in double-intrinsics.ll (which now exhaustively checks double intrinsics).

Refresh patch and fix typo in comment. This patch does not update float-intrinsics.ll as new promotion code needs to be added for FPOWI. I will post a patch to do this now.

Patch refresh, no functional changes.

Refresh patch and use ZExtPromotedInteger.

Updated to apply cleanly without fuzz and to use ZExtPromotedInteger as suggested by Eli.

Refreshing patch so it applies without fuzz. No functional changes.

Confirming this patch still applies cleanly against current LLVM HEAD. Ping?

Ah - I was waiting for this to be committed before returning to review the rest!

This seems a positive improvement and I don't want to bikeshed it too much, but it does feel a bit arbitrary to reject tiny and kernel codemodels in getEffectiveCodeModel. You could imagine having a virtual isSupportedCodeModel. Might be more hassle than it's worth though...

Just a ping on landing this. Are you awaiting additional reviews?

I've re-checked this patch still applies cleanly against current HEAD.

Could you please a double-frem.ll test as well?

Thanks Lewis, this looks good to me - just a couple of very minor requested tweaks noted inline.

Looks good to me, thanks!

Great catch, thanks. I know there's good test coverage for by virtue of it being a common pattern, but could you please add a more targeted unit test to e.g. test/CodeGen/RISCV/i32-icmp.ll? Otherwise, looks good to me.

Thanks Luis, this looks good to me. Just a minor nitpick re OP -> Op.

Rebase and improve testing for double<->int conversion.

Updated to use target DAG combines for the int/float conversions.

Updated to:

• Add missing remw/remuw patterns
• Add exhaustive testing of the *W patterns introduced inrv64m
• Add and make use of sexti32 PatFrags

Updated to fix a PatFrags-related assertion and to add exhaustive testing for the *W instruction patterns. Although verbose, these tests have shown their worth by 1) identifying a missing pattern which I've now added and 2) flagging a few cases where codegen could be improved in future.

Thanks Shiva. I'm updating the patch series in a way that adds much more exhaustive testing for the *W patterns and fleshes out patterns for cases like this. I mused on the RV64I patch review thread whether there was something smarter to do here - maybe some canonicalisation. But regardless of that, it's helpful to have more exhaustive tests.

I have a number of changes implemented while I was travelling, and will update this patch series.

Hi Daliang and welcome to the LLVM community.

The presence of no-op any_extend can cause assertions during the DAG combiner, and there's no guarantee that the no-op any_extend will be visited first. I'll update this to avoid the issue, probably setting a target combiner for any_extend to legalise any_extend + bitcast directly to BitcastAndSextF32ToI64.

In D52961#1261196, @sabuasal wrote:

In D52961#1258464, @asb wrote:

No real reason to prefer one or the other when they're semantically identical. It's mostly to be closer to gas/gcc which uses addi at least for the isInt<12> case.

I see. makes sense. I just checked gcc and it seems to be favoring addi for that case.

Fixed out of date comment (thanks @niosHD).

Update to address review comments (thanks!).

Update to add zexti32 PatFrag and to eliminate unnecessary masking of shift amount values.

Updating patch with additional test. Thanks for the review @efriedma.

Yes, I'll add the appropraite SLLW/SRAW/SRLW patterns in the RV64I codegen patch. Given this improvement isn't specific to RV64I, I wrote this patch to apply against current HEAD.

Thanks for the feedback @efriedma, I've posted a patch that tackles this at instruction selection time: D53224.

Updated patch to add a TODO to indicate that it may sometimes be preferable to load from the constant pool. As Bruce points out, this isn't always a clear win.

The other motivation is that many of our codegen tests are going to include both RV32 and RV64 check lines. It's distracting to see addiw for materialising constants when it's not necessary, and it represents an unnecessary diff vs the equivalent RV32 code.

Rebased patch. No changes, but I had to re-generate the new atomic-cmpxchg.ll lines as regalloc is slightly different after the recent enableMultipleCopyHints change.

Hi Ana, just some minor comments. With tweaks along the suggested lines, this looks good to me.

Shiva and Kito didn't seem to have any concerns so I was just re-checking prior to committing. I had a closer look at getAssemblerPtr and see this method was introduced by @niravd in rL331218. Nirav - are you happy for this patch to change getAssemblerPtr so it doesn't just return nullptr in MCStreamer.h? I don't think gating on UseAssemblerInfoForParsing makes sense in our case.

I'm trying to keep things separate where possible, but yes we will want getIntImmCost and as you point out it's likely to be more useful for RV64. Ideally it would recognise that compressible code sequences are cheaper than non-compressible sequences.

In D52961#1258446, @sabuasal wrote:

The helper makes it clear to follow. Thanks!

I am just not sure why you did the change to prefer add to addiw when for 64bit? is addi faster than addiw?

Thanks for the review, I was hoping there was an opportunity here to avoid the need for backend-specific peepholes. I'll handle in the backend instead. For my own edification, what do you mean by the shift amount being too large to undef?

There's one aspect I'd appreciate some input on. Full coverage of sext/zext/aext inputs and sext/aext outputs would require a lot of repetitive tests. But these tests would have value. e.g. to generate srlw for the following two functions:

Update to attache the correct diff this time.