In D102839#2784585, @kito-cheng wrote:

Personally I prefer to deprecate -mno-div soon, but based on the rule for RISC-V GNU toolchain, it need to wait Zmmul extension frozen.
My plan is deprecate the -mno-div and emit warning to tell user should use Zmmul instead once it frozen.

I'll put this patch on the agenda for the sync-up call today, but particularly now it emits a warning I'm inclined to think we should accept this in order to provide drop-in compatibility for GNU toolchains.

In D70401#2733003, @khchen wrote:

Hi, I would like to add ilp32e ABI support in llvm
Is there anyone working on this?
It seem the one thing missed is ilp32e ABI should disallow D ISA extension.
Is there anything else?

In D100529#2690729, @khchen wrote:

I think using this proxy header for testing seems good.

IMO, maybe we still need to find a way to reduce the compilation time for better user experience.

The GCC torture suite now gets a 100% pass rate for me - thanks for fixing. I've left various minor notes about comment phrasing and formatting etc.

In D98932#2663612, @asb wrote:

It would definitely be good to get a test case for this as Fraser says.

I appreciate it's a bit hard to implement an alternate fix that doesn't involve the risk of regressing other backends, but it's not ideal to remove a call to a hook that really RISC-V should be setting (per the description of setHasMultipleConditionRegisters, this should be set for RISC-V).

This is a good point fix for the immediate problem, but the concern is just that future optimiser changes means there are more issues or missed optimisation opportunities down the road.

If we did want to do this, probably worth adding comments to explain that we'd like to setHasMultipleConditionRegisters ideally, but are choosing not to due to codegen regressions.

@frasercrmck you mentioned this patch doesn't quite seem right to you - is there a particular part you're concerned about. I've spent some time going over both this and D99068. The logic of this patch makes more intuitive sense to me, but that may be as it's a long time since I have a better memory of the logic it's modifying. Though Luis' approach does indeed seem to reduce stack size in some cases, and of course directly mirrors the matching SystemZ fix.

It would definitely be good to get a test case for this as Fraser says.

LGTM, left a couple of nits on comment typos. Thanks!

I think it's OK to land without this, but: Running this patch quickly across the GCC torture suite, the only codegen changes are to 20120817-1.c and pr51581-{1,2}.c on RV32 where a mulhsu is now selected rather than a solitary mulh. I believe this is still correct, but it made me wonder if we are missing some test coverage and should have a codegen test that would demonstrate that change.

Needs a rebase for the test changes to apply, but this LGTM.

Let's mention it in the community call today just in case anyone has strong differing opinions, but this lowering strategy makes much more intuitive sense to me.

Could you please rebase this to account for D98716?

LGTM. Luis is the one who's dived deep into the memory mappings etc, but he and I had a discussion offline to help me understand the reasoning for this change, and given it's also had an LGTM from sanitizer devs I think this is good to land.

LGTM, this seems the right direction to go in.

In D99029#2654346, @craig.topper wrote:

I made a dedicated fix for the motivating issue with 5692fc38e0d17abc55a4a84da98f021a1d53d76d so I'm not as concerned with pushing this patch now.

I've been a bit busy with some none-LLVM things the past few days too, so sorry for the delay in commenting. I'll try and loop back shortly.

LGTM.

Can I just check the reasoning on the naming? I see that the bitmanip 0.93 spec proposes _{rv,rv32,rv64}_{opname} intrinsics. Does the __builtin__{riscv,riscv32,riscv64}_opname format match what GCC are doing / planning to do here? Precedent for RVV, for other archs, or something else?

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Great cleanup, thanks for this.

This is much better, thank you.

@sebastian-ne: to ensure the right people see this, could you file a bug for your problem please? https://llvm.org/docs/HowToSubmitABug.html

Great idea - the RISC-V bit looks good to me.

We discussed this briefly in the RISC-V call as I noted this patchset has been sat open for some time. One thing that might be helpful is whether you could say a little bit more about the goal for this patchset. Once this lands, what's the next step? Is there some relevant RFC, or equivalent changes being made to other in-tree architectures?

I think I may have had the impression from some previous discussions that byval may have limited positive impact, and that letting Clang add the copies to the IR might in some cases help optimisations (that may not be written to reason about byval). You've got a good example of a case where the lack of byval causes weaker optimisation though.

This needs a rebase, but LGTM to land after that. Thanks!

LGTM, thanks!

In D93298#2544459, @StephenFan wrote:

According to @jrtc27 's review that is
"As for Zfinx itself, well, the idea is fine, but I really detest the way it's being done as an extension to F/D/Zfh. Running F code on an FZfh core _does not work_ so it is not an _extension_. Instead it should really be a set of separate extensions to I/E that conflict with F/D/Zfh, i.e. Zfinx, Zdinx and Zfhinx, but apparently asking code that complies with a ratified standard to change itself in order to not break when a new extension is introduced is a-ok in the RISC-V world.".
We split the Zfinx into 3 separate extensions which is Zfinx, Zdinx, and Zfhinx.

I started reviewing this alongside the specification in https://github.com/riscv/riscv-zfinx/blob/master/Zfinx_spec.adoc. At the time of writing, it seems to define "zfinx" but not "zfhinx" and "zfdinx" as seem to be used in this patch. I think intent is that rv32ifd_zfinx is the equivalent of "zfdinx" in this patch. Is there a reason to go for different naming, or a different version of the spec I should be looking at?

Per our discussion in the RISC-V community call today, PLCT Lab + Andes are going to reach out to each other to try to coordinate these MC layer and future codegen patches (thanks!).

Per our discussion in the RISC-V community call today, this patch is intended as more of a "request for comment" at this stage given the P extension encoding issues, and PLCT Lab + Andes are going to reach out to each other to try to coordinate these MC layer and future codegen patches (thanks!).

LGTM modulo one additional request: please update the comment at the top of RISCVInstrInfoV.td to say "0.10" rather than "0.9".

LGTM.

LGTM. It might be worth adding a comment where the patterns were removed to explain we don't have ISel patterns for those instructions because they've been removed from the 0.94 spec (just in case someone starts looking to add them, having not followed upstream development closely).

This was a really nice improvement - thanks Craig!

Hi Jim, thanks for the contribution. I saw you were on the review thread for D94579 which also aims to implement MC layer support for the P extension - could you please comment on the difference between these two patches as you see it?

Thanks for submitting this. It doesn't apply against current HEAD, could you please rebase?

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.