- User Since
- Aug 6 2013, 5:31 AM (237 w, 4 d)
Thu, Feb 22
I've added two suggestions on further tweaking the tests which I think it would be worth adopting. Looks good to me.
Looks good to me now. I had a go at rewording the c_lui_imm comment - feel free to take from that suggestion if you think it's any clearer.
Only remaining concern:
Tue, Feb 20
Sorry for the delay, I've been on paternity leave. Will review this and D43158 ASAP.
Sat, Feb 17
Thu, Feb 8
Ana, can you share the encoded and decoded instruction which causes the qemu crash? This patch shouldn't change the encoding in any way - it should produce the same set of c.lui encodings (all legal), it's just the mapping from logical immediate operand -> encoded form that changes.
Sat, Feb 3
Fri, Feb 2
Thanks Shiva. I've left comments inline regarding the naming convention for this operand type, and the printing policy. As I say, I think we should let c.lui's operand print in the same way as lui for now. Consistently printing hex for lui and c.lui (as I think gnu objdump does) probably makes sense, but lets do that in a separate patch and do it for both instructions at the same time.
Thanks, looks good to me.
Thu, Feb 1
Looks good to me - thanks!
I agree with @RKSimon that splitting the refactoring to a separate patch would make sense. I've previously missed having an update_cc_test_checks.py for IR tests, so think this is a great improvement. End-to-end clang->assembly tests may not be welcome upstream, but given they're trivial to support after this refactoring it would seem mean not to expose that functionality to out-of-tree users who want it.
Adding @modocache as reviewer, who has been responsible for making the opt-viewer tools Py2+Py3 compatible. The changes look sensible to me, but the vast majority of the Python I've written for a long time has been Py3.
Looks like there are two more test files you can update to not pass -mattr=+c to llvm-objdump:
Nice improvement - looks good to me. Thanks!
Hi Ahmed. Many thanks for the contribution, and sorry for the slight delay in reviewing.
rL305083 by @sdardis introduced getRegisterTypeForCallingConv, getNumRegistersForCallingConvention and some other related hooks. These can be used to a similar effect - did you evaluate that approach instead?
I'm liking the look of this, looking forward to giving a final review when you're happy to remove the WIP tag. Thanks!
Hi Shiva. update_llc_test_checks.py basically generates CHECK lines for you according to the RUN lines in the file. This is a fairly minor benefit when writing a test for the first time, but the biggest benefit comes from maintaining the test. When there is a codegen change, test files that contain all instructions are much easier to review.
Tue, Jan 30
My main concern with this patch is that the description doesn't really match what it does. The current in-tree code _doesn't_ call gcc to link for the tested configuration (a multilib toolchain), and this is verified with the tests in test/Driver/riscv32-toolchain.c. D39963 originally added support for a baremetal toolchain, but was changed to focus on the multilib Linux toolchain for a couple of reasons:
- This was a more straight-forward change
- Downstream users such as yourself had a higher priority for building for a Linux target
- It looks like we'd want to discuss the possibility of adding RISC-V support to lib/Driver/ToolChains/BareMetal rather than adding yet another target-specific toolchain
Fri, Jan 26
Sorry, my mistake - the committed version did include the suggested changes.
Hi Shiva. In the future it's worth nothing that if you include the review url in the commit, then Phabricator automatically picks it up.
Jan 25 2018
Hi Shiva, why don't you follow the instructions at http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access and request commit access? Many many thanks again for all of your contributions to the RISC-V backend so far.
Jan 23 2018
Jan 22 2018
Jan 18 2018
Leslie - there's a huge amount here that's copied and pasted from ARMScheduleA9.td, much of it really isn't relevant to any current RISC-V implementation, such as load-multiple/store-multiple and vector operations. Additionally, the A9 is an out-of-order superscalar core. The majority of the RISC-V community are targeting in-order cores (Rocket, PULP, other microcontroller class cores). If you're keen to explore scheduling models for out-of-order cores then BOOM is of course the obvious target.
Hi Leslie. Obviously we need to resolve the CCState issue before merging this, as it would regress codegen in its current state. Good observation that we can get the DataLayout via CCState. Given that, why not get rid of the various wrapper functions and just determine XLen and XLenVT from the DataLayout?
Thanks Ana, looks good to me.
Hi Leslie, this patch includes rather a lot which makes it somewhat hard to review - i.e. you're doing call handling, loads/store selection, selection for some ALU ops, FPR32/FPR64 regs + fcmp, and there aren't any tests attached that check this all works.
Jan 17 2018
Thanks for this - definitely a real problem that we want to fix.
Temporarily unaccepting - sorry, I accidentally added comments and acceptance for a different patch. Comments following shortly.
clang-format shows me a few minor tweaks:
Thanks! Looks good to me.
Hi @martell, do you think you might have time to add a test for this?
Jan 15 2018
Thanks Eli, that saved me some time - fixed in rL322514 (clang-x86_64-linux-selfhost-modules-2 is now green).
The riscv32-abi and risc64-abi tests are failing (specifically the vararg checks) on the clang-with-thin-lto and modules-slave-2 buildbots:
Jan 13 2018
Hi Petr, thanks for the report. This should be addressed in rL322435.
Jan 12 2018
I think the change to "To submit an updated patch:" should probably be left out, given we agree there's no benefit currently in specifying the repo for the diff object (as opposed to the review).
I've addressed all outstanding comments (a number of response are inline).
Hi Mario - as this is marked in WIP I've added a few initial comments rather than given a 100% complete review.
Thanks for the update Shiva - a few minor comments in-line
My suggestion would be to remove the MCSubtarget field (it can be reintroduced when future changes need it), but obviously keep it as an argument to the constructor where it is used.
Hi Shiva. I was just looking at applying this and the STI field is triggering a compiler warning as it's not used: it's initialised in the RISCVTargetELFStreamer constructor, but isn't actually read from anywhere.
Yes, I'm sure the repository information is stored somewhere attached to the diff object - but it seems we don't use it. For this patch, I think the best way forwards is to advise users they can ignore the field when uploading the diff, and should enter it on the subsequent page. It's possible the workflow will be improved based on your upstream bug report, but we'd have to wait for the LLVM phabricator install to be updated anyway.
Jan 11 2018
Well spotted. I've just verified that if you enter the repository on the first page it isn't automatically copied through to the "Create Revision" page. I'm assuming that it's setting the repository on the "create revision" stage that results in automatically adding llvm-commits of cfe-commits to the subscriber list?
I've just had a painful time landing D39963 due to failures on the Windows buildbots. I think you'll have problems on Windows with the tests in this patch, as Windows surely won't default to /usr/bin/as. Sadly I don't have access to a windows Clang build to confirm what path it will default to.
Thanks Ana for outlining your current thinking. Without reviewing the fine details, this seems in the direction we were all discussing.
With a couple of minor nits, looks good to me. Thanks as always Shiva
Looks good to me!
Am I correct in thinking that as long as the repository field is filled out, there should never be a reason to explicitly add llvm-commits or cfe-commits to the subscriber list? If so it would probably make sense to remove the note "subscribe mailing lists that you want to be included in the review. If your patch is for LLVM, add llvm-commits as a Subscriber; if your patch is for Clang, add cfe-commits." Instead, you could expand the bullet point on the repository field to say "Entering the correct repository will ensure that the appropriate mailing list (llvm-commits or cfe-commits) is automatically subscribed to patch patch updates.