- User Since
- Aug 6 2013, 5:31 AM (246 w, 1 h)
Thanks for the updates Shiva. I've added a few more suggested changes, and I think we're almost there.
Thanks Shiva. I think just two outstanding issues. Some of the test files in test/CodeGen/RISCV that weren't generated with update_llc_test_checks.py have unwanted changes in this patch. With that fixed, this is looking good to me.
Thu, Apr 19
Hi @spetrovic - I think Hal Finkel's earlier request to update the comments of IsBetterAsSingleFieldRun remains unaddressed. It looks like at least the comment string immediately before auto IsBetterAsSingleFieldRun needs to be updated to reflect the changed behaviour.
This is looking great, the only remaining code comment I have is that getExtensionFeatures needs a comment describing it.
Hi Shiva. Thanks for this, I think the best way of landing this is to split the MC and codegen changes. Could you please:
- Make a new review that contains only the changes required to support call foo in the MC layer
- Rebase this patch on top of that, and update the title to something like 'Expand function call to "call" pseudoinstruction'
Thanks Sameer. This is definitely a win for a range of inputs, but I'm also seeing regressions. An extra instruction is now required for any standalone global access + offset. There are even some cases where the global access is going from 2 instructions to 4 instructions (global with very large offset, requiring lui+addi). Do you have any thoughts on addressing these cases?
Wed, Apr 18
In the end I decided to temporarily revert this patch. I've committed patches that fill in the holes identified in testing, and added the straight-forward patch for imm32 with lo12=0.
Ok, I've had a good think about this issue. I was slightly over-eager in committing this last night. Something like PseudoLI seems necessary for the more complex materialisation logic required for 64-bit immediates in RV64, but we can do without for RV32. I've weighed up whether to revert and revise, or to make changes post commit
I'm just working through this now. I'll play around with the options and update this thread within the next few hours.
Tue, Apr 17
Thanks again Mario. I've reviewed the new RISCVISelDAGToDAG changes and just have a minor comment. This is also looking good when testing with the torture suite. I'll commit this as soon as you can confirm my minor query.
CC @sdardis - micromips seems to use aggressive linker relaxation in binutils ld meaning this may have some current or future relevance to the Mips LLVM backend.
Mon, Apr 16
Could you please expand rv32-relaxation.s and rv64-relaxation.s so the test includes the behaviour for branches to an unresolved symbol?
Thanks Kito. -mrelax and -mno-relax currently only affect the backend. For completeness, I think this patch needs to pass the appropriate flag to the linker depending on relax/no-relax.
Thanks, that's definitely an error. Please do feel totally free to commit straight-forward trivial fixes like this without the need for pre-commit review.
Fri, Apr 13
I'd do the compressed changes in a different patch. Thanks for updating the peephole RISCVISelDAGToDAG, I'll review that bit ASAP and then commit. At a first look, it seems to handle this exactly as I would expect.
Thu, Apr 12
Thanks! Looks good to me.
I think this patch and its tests would be best if based on D44886, so it can be clear which behaviour is conditional on linker relaxation being enabled.
Given that Simon's D45181 will be dependent on this (as it should really query whether linker relaxation is enabled), it probably makes sense to get this merged sooner rather than later.
Looks good to me, just missing help text on the command line options.
This seems like a sensible intermediate step, even if it's not a 100% match for gcc behaviour.
Thanks Shiva. This looks good to me - just two additional minor suggestions prior to commit (add a TODO comment and switch to using dyn_cast in one place).
I've added a few comments on tweaking the error messages based on your tests.
Thanks, looks good to me.
Thanks Kito, looks good to me.
Wed, Apr 11
Fri, Apr 6
Thanks, this looks good to me now.
Looks good to me. Thanks.
Thu, Apr 5
From a quick look in binutils, it seems micromips could be tripped up by similar issues.
Thanks for this Simon.
Could you please add tests that demonstrate the new logic in shouldForceRelocation? Specifically, it would be good to show that relocations are emitted even for local control flow, when they weren't before.
Could you please add a test? Given that the current version of D44886 enables linker relaxation by default in the backend, shouldn't -mno-relax cause -relax to be set?
Thanks for this as always Shiva. Mostly minor comments or questions in this review.
Thanks for the test changes.
Based on Andrew's response (thanks Kito for sending the query) it looks like GCC accepting lowercase only is intentional, and we should follow that. In which case, it might be an improvement to reject uppercase letters in the ISA string with a message saying that only lowercase letters are accepted.
Wed, Apr 4
Thanks, this looks good to me.
Thanks Eli for the info on hasFeature.
Could you please add a test, perhaps to test/Driver/frame-pointer.c?
This seems like a good improvement. As Eli says, avoiding the virtual function if possible would be ideal.
Thu, Mar 29
This diff seems to have accidentally picked up changes to a number of tests.
Wed, Mar 28
Could you please make a new review with your proposed fix(es) so we can continue discussion there? It's much easier to keep track of things that way. Typically one phabricator review == one commit.
Could you please post the change as a separate revision, as a diff vs what was committed?
Mar 23 2018
Mar 22 2018
Thanks Mario. I think this is looking good to land now.
Thanks Kito. I've added some comments inline.
Thanks for this Kito. A tiny formatting nit, but otherwise this looks good to me.
Hi Mandeep, thanks for the patch. This looks good to me, but there are just two changes I would suggest prior to committing:
Mar 21 2018
Mar 20 2018
Mar 19 2018
Mar 15 2018
By the way, I'm getting the following error while trying to build this:
Mar 14 2018
Thanks for submitting this Kito. I've added some minor in-line comments. It might also be worth adding a couple of extra cases to the tests:
- Repeated letters in the ISA string (e.g. rv32immafd)
- Upper case letters in the ISA string. We currently reject these (as does GCC). It would be worth having a test that tracks this behaviour
Thanks for pursuing this issue on the binutils bug tracker. Might you be interested in proposing a patch to the psabi doc that clarifies the requirement here? It seems that the main reason for forcing this behaviour is for linker relaxation - I hope we can co-ordinate with binutils to allow addends when linker relaxation is disabled.
And I have to apologise again that it has taken longer than expected to get back to you on this. I hope my slow feedback this time won't put you off future contributions - going forward, things should be much faster.
For reference, this was discussed on llvm-dev here: http://lists.llvm.org/pipermail/llvm-dev/2018-March/121634.html
Thanks for making those changes Shiva . The fact we can simplify determineFrameLayout is a good observation.
Mar 13 2018
Mar 8 2018
Actually, looking again I'm not having any problems building just the RISCV backend even without this patch. e.g. an invocation like the following seems fine:
Hi Azharuddin, many thanks for the contribution. I hadn't spotted this part of LLVM's CMake build system, and it's definitely useful to be able to build just the RISCV backend. I've tried this locally, and the problem I'm seeing is that a good number of Clang and LLVM tests fail when you don't build X86 support. On my system, 351 unexpected failures across Clang+LLVM. This is a problem with the tests rather than this patch (they should really be disabled if there is no X86 support), but it does limit the usefulness of just building _only_ the RISC-V backend. Is it expected that you build an LLVM with X86 and RISCV, ensure 100% tests pass, then build with just the RISCV backend and ignore any new failures?
Mar 2 2018
Looks good to me. You might want to use c.nop in the tests rather than nop to avoid test churn (changing offsets) once automatic compression is merged.
Now this is decoupled from the other changes, it seems good to land whenever you're ready.
Mar 1 2018
The extra sp adjustments introduced in calling-conv.ll and vararg.ll seem unnecessary - are they?
Hi Sameer. Some notes primarily from reviewing the patterns and testing so far:
- In the RISCV backend we order tablegen instruction definitions to match the ordering in the instruction set manual (which allows easy cross-reference, makes it harder to miss one) and typically order codegen patterns grouped by function. For the case of the CompressPat, I'd probably keep them ordered by the instruction set manual (page 82 in the spec, like the instruction definitions) as each pattern is so mechanical. This makes it easier at a glance to see that every compressed instruction has a matching transformation. The basic MC tests should be ordered to reflect this too. I know this is probably a tedious change to make, but I do think a little effort in the formatting here has an impact on maintainability.
- Related to the above, grouping the 'commuted and repeated patterns' after the normal patterns makes it easier to miss one and a little more effort to check it's there. I'd always group patterns for the same target compressed instruction together.
- Are the CHECK-BYTES lines worthwhile? I'm fairly happy making the assumption that the encoding printed by the InstPrinter is reflected in the output object (or at least, I'm not sure it's worth adding an extra line for every single check to ensure this)
- For other MC tests, we've checked the instructions common to rv32 and rv64 in a single file. See e.g. rv32c-valid.s, rv32c-only-valid.s etc
- This compression transformation is intended to use the same code path to support a range of use cases: .s->.o, .ll->.o, showing aliases in disassembly, compression of inline asm. This patch only contains tests for the assembler and disassembler paths. Given that the compression transformation is implemented as an MCInst->MCInst transformation, it's fine for the exhaustive tests to live in test/MC/RISCV, but we should at least have some sanity checks that the codegen compression path is working (including with inline asm).
Thanks for adding the test case, I added a few minor comments there.
Feb 28 2018
Adding llvm-commits as a subscriber. A couple more comments inline