- User Since
- Aug 6 2013, 5:31 AM (336 w, 3 d)
Tue, Jan 14
Mon, Jan 13
This looks good to me, thanks James. I had a closer step through of the logic here to convince myself.
Fri, Jan 10
Thanks for the fix! As Sam says, explaining the issue in the commit message would be useful.
Thu, Jan 9
Thu, Dec 19
Dec 16 2019
Please update the check so it checks the error location too, like we do for other *invalid.s checks.
Dec 12 2019
Dec 5 2019
Nov 21 2019
I note that the TargetFrameLowering hooks canUseAsPrologue and canUseAsEpilogue are both called by the shrink wrapper. They default to true, but targets may need to override this for correctness. Looking at e.g. AArch64, I see it overrides canUseAsPrologue and returns false in the case that the function needs stack realignment and there are no scratch registers available. Are you certain that no such case is needed for RISC-V?
LGTM, thanks Luis.
Thanks Simon, LGTM. I did wonder about just saying e.g. "the 'c' instruction set extension", but on balance I can't see it being any easier to understand or more obvious what to change, and it's handy giving a longer description.
Nov 15 2019
Compared this to commits that introduced assembly mnemonic spellchecking for other backends, and it looks good to me. Thanks Simon!
Nov 14 2019
Please update the commit message to clarify the cases where we do deviate from the GCC defaults, but this looks good to me.
Oct 31 2019
Thanks James - won't this still leave problems for structs that need flattening?
Oct 24 2019
Oct 14 2019
Oct 3 2019
Thanks Ed, could you add some tests to test/MC/RISCV/rvf-aliases-valid.s please?
I think this patch is orthogonal to save/restore via libcalls, and we can probably land it sooner. Could you please rebase so it's standalone?
@schwab - do you need someone to commit this for you?
I like the direction of this, Thanks Ana. Did you look at modifying getInstSizeInBytes? That would benefit other callers beyond the outliner.
Sep 26 2019
Plus our dev meeting coding lab last year stepped through a similar optimisation, so we really should land it.
This LGTM, thanks. Added a few nits. I think Sam's suggestion of adding a helper function or two to handle the register arithmetic is worth evaluating, but doesn't need to be done in this patch.
Sep 17 2019
Sep 11 2019
Sep 10 2019
Aug 29 2019
Thanks for pushing this forwards Sam. For the RISC-V backend we've been using a downstream shell script for this for ages, and I've always felt it would be better if the torture suite could be used from the LLVM test suite. It's been a while since I went over the excluded tests, it's possible it would benefit from further review if all the masked tests really should be masked.
Aug 27 2019
This LGTM, but given how much discussion there has been about MaxPromoteWidth it would be great to get some test coverage for it.
This triggers when building the Linux kernel too. I forgot to submit this patch after getting that working. I also couldn't get a meaningful test case for it that didn't seem like it would be horribly brittle. LGTM, thanks.
Aug 20 2019
Aug 19 2019
Resigning as a reviewer, as the intent of this change is captured in rL366327.
Looks good to me, thanks! We may later review whether the ability to enable/disable these hints is worth it or not (I know I'd been in favour from the start, but looking at it with fresh eyes I'm not sure it's fully necessary), but this is a useful step forwards.
Actually changing my mind. This would be better tested by adopting something like arch-reg-names.s from D66139 (and extending that to handle FP regs), and leaving the inline-asm tests alone (huge increase in test size, which makes them harder to follow). Make that change, and then it looks good to me.
LGTM, but wouldn't it be worth moving arch-reg-names.s to D65950, and also expanding it to handle FP regs?
Aug 9 2019
This causes failures on the GCC torture suite:
Aug 8 2019
Also, clang-format seems to prefer to format some of this patch differently - worth running it through again.
Please to submit a follow-on patch for targetHandlesStackFrameRounding
Aug 6 2019
Many thanks for the patch. Could you please add some tests for this behaviour? I imagine you'll want to add a new directory in test/Driver/Inputs with a Debian tree skeleton. See D63497 for a relevant example.
Aug 1 2019
Thank Roger. While we're doing this, I think it would make sense to default to ilp32d on 32-bit Linux? I know the glibc support etc is less mature than for RV64, but it seems the sensible thing to do.
Adding explicit LGTM in the Phabrictor UI. Thanks again Lewis!
Thanks for looking at an alternate approach here James. In light of the better error messages I'm going to go with D65205 at this point, but I'm open to future refactoring or improvements. I haven't spent a lot of time considering it, but my initial reaction on PushParens is I feel it might make the parsing logic harder to follow in return for a fairly minor reduction in repetition. But I could be convinced otherwise...
Looks good to me - thanks!
Added a nit around a doc comment, but this is looking good - thanks Sam. I agree that the error messages are nicer in this patch vs D65291 for now. However I'm very open to further improvements and refactoring of AsmParser logic in the future in follow-on patches. Only caveat to that is that the asm parser for any backend isn't particularly pretty, and that sometimes a little bit of repetition can be easier to read than an all-singing-all-dancing function that can do anything based on what function args you pass.
Jul 31 2019
Getting sufficient LGTMs is a bit of a judgement call with LLVM. If the change is large and invasive, then typically you'd wait for the OK from the relevant code owner. For something like this, totally find for anyone active in that part of the codebase to approve.
Jul 26 2019
This looks good to me, thanks! Upon review I considered whether evaluateConstantImm should always set VK, but it seems sensible to rely on the caller to initialise it if necessary, as setting VK when evaluateConstantImm failed seems surprising.
Jul 18 2019
Looking at this in more detail - shouldn't we be making use of the R_RISCV_SET* relocations?
Jul 17 2019
LGTM, thanks. I've spent quite a bit of time comparing output vs gcc so I'm pretty confident here now. I committed a slightly different test strategy in the DWARF encoding work I just committed, so will adjust this patch to build on that.
Jul 16 2019
Hi Ed, this is really hard to review without any documentation in the psABI or otherwise. Can you confirm that you've verified this matches gcc/gas? Are you planning to submit document to riscv-elf-psabi-doc to reflect this?