Thanks for extending the test coverage!

LGTM, and I agree with the reasoning re preferring to point users towards zbb.

LGTM, added two minor notes.

LGTM, going with the 0.94 deconflicted naming seems the only reasonable thing to do https://github.com/riscv/riscv-bitmanip/pull/102/commits/b962f8a04be570a93c3c4788425ee2e8a14e9c56

In D94652#2512063, @frasercrmck wrote:

In D94652#2509956, @craig.topper wrote:

My hope is to get the 0.93 move into LLVM 12. Zbb is marked frozen in the 0.93 spec and does not include these. So I'd at least like them out of Zbb. Would it be better to just remove them until they have a home?

Mm yes, if Zbb is frozen and doesn't include them then I agree they need to go somewhere. Removing them sounds a bit drastic. If the release branch will be created in 6 days, how long can we delay the decision?

Not a blocker as I think this is just something that bitmanip has been less complete with vs the standard extensions, and you're just the last person to touch this, but we should probably be testing that rv32zba rejects rv64-only instructions and that rv64zba accepts the rv32 instructions. See rv64f-valid.s and rv32f-valid.s.

Straight forward change, LGTM. It seems there's nothing contentious in the patchset, so I'd expect the series can land basically all at once, to minimise the time we support a weird 0.92/0.93 hybrid.

These LGTM. This patch should presumably be marked dependent on D94637.

I don't think any of the other patches in the stack update the comment at the top of RISCVInstrInfoB.td to say "version 0.92" rather than "version 0.93", and this is probably a reasonable patch to do it in.

@kito-cheng could you please confirm that this patch handles sub-extensions in the same way GCC does. i.e. -march=rv32izbb0p92 defines __riscv_zbb but NOT __riscv_b? That seems logical to me, as otherwise it would be cumbersome to check if the whole extension is supported rather than just a subset, but I just wanted to confirm.

Nitpick: this would be better titled "[RISCV] Add intrinsics for vector AMO instructions" - I was a little confused seeing the title come past :)

In D94568#2495052, @kito-cheng wrote:

RVB 0.93 is an awkward version to me, there is mnemonic conflict which is not resolved during release process since it's kind of too rush, the conflict one is bext in zbe and zbs...

However it's also a milestone for B-ext, since this version claim zba, zbb and zbc is frozen, maybe those 3 sub-ext. could be removed from the umbrella of -menable-experimental-extension ?

@asb What do you think about this?

I think this is a reasonable change. The pattern of having "Utils" is common to AArch64, AMDGPU, ARM, and RISCV but I agree with your point that it's not cleanly separated from MCTargetDesc, so there's little benefit.

Interesting. So to summarise my views on pros/cons

It's great to be able to drop those ugly patterns - nice cleanup. Thanks.

This needs a rebase, but otherwise looks good to me. Thanks Sam!

In D92715#2443376, @craig.topper wrote:

Can we discuss this patch in tomorrows RISC-V meeting? @jrtc27 @kito-cheng @khchen @liaolucy

I'm afraid this causes compile-time failures with 20030323-1.c, multi-ix.c, and pr53645.c.

I think moving NODE_NAME_CASE(foo) to the left so it's aligned where the case statement would be (same as the equivalent in X86ISelLowering and AMDGPUISelLowering) would be slightly better. I wouldn't be opposed to using // clang-format off to stop clang-format from reformatting (though it seems more common in LLVM right now to just ignore clang-format's preference).

Ouch - I'm sorry to have contributed to the bloating of LLVM binaries!

This seems like a good cleanup to me - thanks.

In D91199#2389105, @craig.topper wrote:

In D91199#2388012, @frasercrmck wrote:

LGTM; from a quick look I can't see anything that would need that glue. Do you have an idea why it was added?

No. As far as I could tell it had been there but not used since the first patch that added RISCVISD::SELECT_CC.

Historically for the RISC-V backend we've split patches into the MC layer changes and the codegen changes in order to make them easier to review. Do you think you'd be able to do the same here?

Thanks Roger, this looks good to me and doesn't seem to break anything in the GCC torture suite at least.

I think there's a little more to supporting older names than just the date of the spec. I agree that we don't expect anyone doing serious RISC-V work to be working to the 1.9 spec (unless there are cores taped out with that spec we don't know about), but in cases where there have been CSR renames it's often the case that people migrate to newer specification versions but don't always change the naming. There's an argument for the compiler complaining at them in this case, but there's also the user experience issue. It looks like the RISC-V compliance suite for instance still uses the sptbr name https://github.com/riscv/riscv-compliance/issues/107

This LGTM. @lenary are you happy your comments are addressed?

LGTM, thanks Luis.

LGTM, thanks Paul.

I think once @jrtc27 confirms all her issues are addressed this is good to land.

LGTM, thanks.

It looks like others are reviewing the implementation details, but I just wanted to chime in to say that this looks like a useful feature for the RISC-V backend too. Currently we set CostPerUse for some registers in order to tweak register allocation in a way that benefits codegen when the compressed instruction set extension is enabled. Although the impact is _very_ minimal, this slightly harms targets that don't support the compressed instruction set (admittedly, this isn't really true of any shipping RISC-V processor so not a big concern). It would be good to use this vary the CostPerUse depending on target RISC-V features.

No objections accepting from the RISC-V side. I don't have a set of execution tests for the fshl/fshr changes to validate them, but I didn't see anything obviously wrong. The changes primarily affect the (still experimental) bitmanip extension also, so the bar for review is somewhat lower. The change does seem to improve srliw lowering in a few places in the GCC torture suite as well (in a way that is both correct and beneficial).

The description of isNotDuplicable in MachineInstr.h is:

/// Return true if this instruction cannot be safely duplicated.
/// For example, if the instruction has a unique labels attached
/// to it, duplicating it would cause multiple definition errors.
bool isNotDuplicable(QueryType Type = AnyInBundle) const {
  return hasProperty(MCID::NotDuplicable, Type);
}

I don't think this obviously applies to CSRs, and the property doesn't seem to be applied to instructions that modify control registers for other in-tree targets.

Thanks for spotting this one - LGTM.

@jrtc27 are you happy with this patch in its current form or are there outstanding issues to be addressed?

@jrtc27 are you happy with this patch now? Thanks for your review on this and thanks @msizanoen1 for providing this fix.

In D79870#2153608, @simoncook wrote:

It's a shame this just missed the creation of the llvm 11.0 branch, do we think it's worth trying to get this backported since it only just missed?

LGTM, thanks!

I'm getting a test failure on rv64-relax-all.s with this patch?

Hi @jrtc27 - it would be nice to get this in before LLVM 11 branches. Will you have time to add the extra comment and commit today?

This looks good to land IMHO. It's a nice improvement for RISC-V, and it seems to have been sufficiently reviewed by people active with the DAGCombiner. Thanks Roger.

LGTM, thanks!

I've added some suggestions to clarify the code comments. I think before landing it would be good to address the crash Sam pointed out for an invalid -march, but otherwise I think this looks good to me (at least, it seems worth landing this and if further issues crop up we can fix them and request them for merging into the LLVM 11 branch).

This broke the build for -DBUILD_SHARED_LIBS=True. I've committed rG5282a6186c which fixes it for me.

Let's go for it! Thanks.

This LGTM from a RISC-V perspective. I'll likely follow up with a RISC-V test case similar to the SystemZ one post-commit, but given this is really fixing a cross-platform ABI issue this seems non-urgent. Thanks for spotting and addressing this issue.

Got it, thanks. In that case LGTM and please tweak the commit message (the last paragraph specifically) so it's clear that the two changes are interlinked.

This looks good to me, good catch. I do see a codegen change on regstack-1.c from the GCC Torture Suite, so it might be worth having a quick look to see if it's easy to make a test case based on that.

This is a nice simplification, thanks. My only request before committing is to split out the RISCVTargetMachine to a separate pass, as that is logically distinct.

LGTM, +1 on adding a comment to the expansion functions noting the need to update getInstSizeInBytes. Thanks!

I've gone through and can't see any obvious issues. I defer to one of the RISC-V Vector extension usual suspects for giving a LGTM on the detail of the altered instructions etc. Once we have that, this looks good to land IMHO.

This has been hanging around for a while, but I think we'd basically agreed this is the right logic. The comments have ended up referring to flags that don't exist on Clang making it a little hard to follow, and I've added a request to slightly expand testing. If you make those cleanups I think it should be ready for a final review and merge.

Looks good to me, thanks for catching this.

In D69987#2079524, @rogfer01 wrote:

The patch as it stands now LGTM and I think it can be committed. Is there any objection remaining?

Any further comments @simoncook @asb?

Thanks, this looks good to me. I wasn't aware of MakeLibCallOptions and IsSoften - I think I've wanted something like that before.

In D79155#2088191, @efriedma wrote:

Please add a comment explaining what OffsetInRecord means; then LGTM.

Ping on this. The patch still applies cleanly against current HEAD.

I think there's value in adding these tests (I note libatomic in GCC has a similar set of simple functionality tests), and this seems a sensible way to do it.

Disclaimer: I haven't swotted up on these constrained intrinsics to review the proposed lowering yet, but added a quick note on the setOperationAction calls. I agree with Luis the test cases would probably be easier to read if using the hard float ABI. I think you're lacking any test coverage for f32 as well.

Sorry for the delay on this - the lockdown situation is really hurting my review time, though it looks like my childcare situation will improve from the week after next.

Good catch, thanks for the fix! The logic was incorrectly written assuming isFloatingType would return false for complex values which is of course incorrect.

Please check D79928 which cleans up the visibility of all of these overridden methods.

I'm in favour of merging a patch that is functionally correct and makes incremental improvements. I'd rather avoid any regression in codesize though if possible. How feasible would it to be to avoid that case? It may be worthwhile leaving some of the materialisation improvements for a follow-up patch in order to land the clear wins now.

Thanks, this looks good to me. PreSched2 is logically a better place for the standard pseudo expansions, though I'm not seeing any codegen changes at all with e.g. the GCC torture suite. But I'm happy to land this as-is.

Could you rebase please? This isn't applying cleanly for me on current master.

Looks like this landed while I was composing the below:

Hi Paolo. I'm sorry this has been left hanging for some time. On the one hand, with this being an experimental feature and purely additive the bar for merging is slightly lower than e.g. a rewrite of all our existing codegen patterns (which could cause new regressions). On the other hand, this pre-commit review is realistically going to be the time when the codegen patterns and associated tests get most scrutiny and it would be a shame to skip that.

Thanks, this is a good improvement. Two thoughts, mainly for discussion rather than blocking issues:

Thanks Sam. I left a few minor comments inline. From looking at the diff in compiler output on the GCC torture suite, I do see a few cases where there is a potential regression. e.g. loop-ivopts-2.c, which represents a case where the stack adjustment is large, but doesn't overflow the 12-bit immediate fields of addi and s[d|w]. The prologue and epilogue are both two instructions and become three instructions with this patch (although it is a code size win).

Just to understand the current status with respect to GCC, am I right that support for these attributes is not in upstream GCC? @kito-cheng is there any plan to upstream?

LGTM, thanks. Left an inline comment pointing out a TODO that can now be removed thanks to this.

LGTM, thanks. Left one tiny nit inline.

In D78907#2012362, @lenary wrote:

In D78907#2012344, @asb wrote:

LGTM. Minor nit: using %0 and %2 as the only two named values seems odd though? Most other tests in test/CodeGen/RISCV use a/b/... for arguments and %0/%1/%2/... in the body.

I generated the testcases in Clang (actually using godbolt, but it's doing the same IR lowering as clang HEAD is), which is why the arguments don't have names. I can re-introduce them if that's easier.

LGTM. Minor nit: using %0 and %2 as the only two named values seems odd though? Most other tests in test/CodeGen/RISCV use a/b/... for arguments and %0/%1/%2/... in the body.

LGTM, thanks!