- User Since
- Nov 21 2016, 2:12 PM (147 w, 6 d)
Thu, Sep 19
Address Review Feedback
- This patch no longer contains the tests, to make review easier. It contains all required licencing files, and the build configuration
- Updates to build configuration based on review feedback and extensive triage
Thanks for the review!
We discussed this in the RISC-V meeting on 19 Sep 2019.
Two nits, that I wanted to submit before the meeting, but didn't get around to.
We discussed this in the RISC-V meeting on 19 Sept 2019. @apazos says there are some SPEC failures in both 2006 and 2017, which would be good to triage.
We discussed this at the RISC-V meeting on 19 Sept 2019. @apazos says she is willing to test this on SPEC.
Tue, Sep 17
LGTM. Please wait for a review from @asb before landing this.
Two small nits, other than that it's looking pretty good.
I have some nits about explicit comments for arguments and default argument values for backwards compatibility. Other than that, it looks like a nice code cleanup.
Noted about the toplevel LICENCE.TXT - I shall update that shortly (hoping not to invalidate the links below).
Mon, Sep 16
One suggestion, then LGTM
Fri, Sep 13
This is looking good. I have just one question this time.
Thu, Sep 12
Great! This looks good to me! I'm glad we're getting rid of all those mov a0, a0 (which are really useless addi a0, a0, 0).
Looks good to me!
- Update Location Copy Code
@schwab Sorry there's been a delay reviewing this. I think it's probably fallen down the crack between "people who are familiar with RISC-V", and "people who are familiar with the sanitizers".
Can you add a test for riscv64-unknown-linux-gnu? I think RISCVToolchain.cpp is only used by the riscv64-unknown-elf target, but I could be wrong.
Ok, I'm happy with the test changes in this patch. Let's get it landed!
Wed, Sep 11
Nice, this is looking a lot better.
Tue, Sep 10
Rebase after landing D65950
Thanks for the patch! I have two comments, which I think will help improve this patch.
Three nits about changes to the testcases that go beyond just reordering.
Is there a way to write a test for this? I realise any assembly goes through the parser, so will be caught before it hits this code. Is there another way of making this work? a MIR-based test?
This was discussed on the LLVM-Dev list: http://lists.llvm.org/pipermail/llvm-dev/2019-September/134890.html
LGTM, now I've looked at how LLVM itself supports code models. I don't mind if that TODO is or isn't deleted.
Nice, I like this new approach! One naming nit, but overall I think this is much better than the first version of the patch.
Mon, Sep 9
- Use -M no-aliases in new tests
Address review feedback in D65950
- That patch now adds numeric-reg-names.s, and we now just update the LLC invocation at the top of the file.
- This patch now adds no tests of its own, it just updates all tests to use the -M options.
Tests updated, they're now a more reasonable size. I'll need to update D66139.
Address Review Feedback:
- Use numeric-reg-names*.s, based on arch-reg-names.s from D66139
- Revert changes to inline-asm-*abi-names.ll
- Rebase changes
I am happy with this. I think this hook is the correct way to go about choosing how to do this optimisation, and accurately conveys what's going on.
I think my feeling is that this patch can land and we can change the default abi for baremetal targets in a follow-up patch.
Wed, Sep 4
This diff is huge, apologies.
- Remove unused file
I have updated the patch to import the testcases, as this seems to be the
consensus on llvm-dev about what to do.
I don't quite understand all the details of this patch. I understand reserving registers that the compiler would otherwise be using as general-purpose registers.
Tue, Sep 3
There are still failures I need to address in:
- MultiSource/MultiSource.Benchmarks.Trimaran.enc-pc1/enc-pc1.test (this is very odd, some numbers are printed as 2048 less than they should be, usually turning them negative)
- MicroBenchmarks/MicroBenchmarks.XRay.FDRMode/fdrmode-bench.test (not investigated)
- MicroBenchmarks/MicroBenchmarks.XRay.ReturnReference/retref-bench.test (not investigated)
@jrtc27 your input here would be valuable.
Ok, I'm happy with this patch and your explanation.
I have two comments/queries below.
Backported to 9.0 in rL370181
Mon, Sep 2
Sat, Aug 31
Fri, Aug 30
- Update subdirectory logic in CMake
- Update test blacklist (using x86 for main blacklist)
- add notes on riscv blacklist
- take into account dg-additional-options
Thu, Aug 29
- Update gcc-c-torture README
Yeah, I need to review the list of tests to skip, which is on my list to do. I will probably initially review that list by testing on the x86-backend, and then letting other backends add their own lists of tests to skip (as we already have for the riscv backend).
- Ignore target-specific dg-options
- remove support for compile-only tests (LNT wants to run things)
- Target-specific Files to Skip
- set lit config.single_source
- Better support for dg-options
- Clarify rev in README
Wed, Aug 28
- Support dg-options and compile tests properly
- Document checkout path
Tue, Aug 27
Address review feedback:
Please may you explain a bit further why calls using varargs (when not passed by the stack) are allowed to be tail-call-optimised?
LGTM with one question:
Aug 22 2019
@jyknight I hear where you're coming from. I'll see what I can do about the psABI document.
Cross linking to the relevant psABI pull request (still pending): https://github.com/riscv/riscv-elf-psabi-doc/pull/112
Update MaxAtomicPromote width to treat it like an ABI feature, and set it to 128
Aug 16 2019
Upon further thought, I realise that MaxAtomicPromoteWidth should be set to 128 regardless of whether a target HasA. I will be updating the patch with the new width and conditions early next week.
Chatted to @asb and he wants me to take over this set of changes.
I'm pretty happy with this. Just one question to answer (below) and then it can probably be merged.
Given this is an ABI-compatibility issue, I've been looking at how GCC and Clang differ in how they deal with issues around size and alignment of atomic objects.
Aug 15 2019
Address review feedback
- Update flag description
Am I correct in thinking that this functionality has already been upstreamed into LLD, and that this patch can now been abandoned?