User Details
- User Since
- Aug 6 2013, 5:31 AM (389 w, 3 d)
Today
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
Yesterday
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.
Thu, Jan 14
Nitpick: this would be better titled "[RISCV] Add intrinsics for vector AMO instructions" - I was a little confused seeing the title come past :)
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.
Dec 10 2020
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!
Nov 26 2020
I'm afraid this causes compile-time failures with 20030323-1.c, multi-ix.c, and pr53645.c.
Nov 13 2020
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).
Nov 12 2020
Ouch - I'm sorry to have contributed to the bloating of LLVM binaries!
This seems like a good cleanup to me - thanks.
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?
Oct 15 2020
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.
Sep 17 2020
I think once @jrtc27 confirms all her issues are addressed this is good to land.
Sep 3 2020
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.
Aug 21 2020
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).
Aug 20 2020
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.
Aug 6 2020
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?
Jul 30 2020
@jrtc27 are you happy with this patch now? Thanks for your review on this and thanks @msizanoen1 for providing this fix.
Jul 21 2020
Jul 16 2020
LGTM, thanks!
Jul 14 2020
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.
Jul 9 2020
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.
Jul 8 2020
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.
Jun 30 2020
Looks good to me, thanks for catching this.
Jun 25 2020
Jun 18 2020
Thanks, this looks good to me. I wasn't aware of MakeLibCallOptions and IsSoften - I think I've wanted something like that before.
Jun 12 2020
Jun 10 2020
Ping on this. The patch still applies cleanly against current HEAD.
Jun 9 2020
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.
May 28 2020
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.
May 19 2020
May 14 2020
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.
May 12 2020
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.
May 11 2020
Looks like this landed while I was composing the below:
May 7 2020
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.
May 6 2020
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.
May 5 2020
LGTM, thanks. Left one tiny nit inline.
Apr 30 2020
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!