- User Since
- Aug 6 2013, 5:31 AM (284 w, 21 h)
Mon, Jan 14
Sat, Jan 12
Patch refresh, no changes. Ping?
Rebased (no meaningful changes).
Fri, Jan 11
Updated. Now handles cmpxchg, supporting all RV64A. The code changes are actually pretty minimal so I hope this is fairly straight-forward to review.
Thanks for the review @efriedma. Updated to add the suggested regression test.
Many thanks to @efriedma for catching the issue with one of the sdiv patterns. I've removed that pattern, and added similar logic to that used for udiv/urem so that anyext is converted to sext if this is likely to result in one of the *w instructions being selected (and thus avoiding unnecessary sext/zext of the input operands).
Thu, Jan 10
I think we've landed enough pre-requisites that this this is good to go in, but it introduces failures to option-pushpop.s and option-relax.s. Can you take a look please?
I know this isn't a behaviour change vs the previous bare_symbol, but I note that GNU as currently accepts call with constants, e.g. call 1234. Should we be doing the same?
Updated to address review comments from Eli (thanks!).
Previous patch was the pre-clang-format version. This update fixes that!
Thu, Jan 3
Updated to improve pattern definitions. The first patterns were over-constrained, as they would only match shifts where the shift amount operand matched the zexti32 pattern. I introduce the shiftwamt PatFrags so it will match a zexti32 shift amount (and hence avoid generating the unnecessary zero-extend), or a shift amount that isn't zero-extended.
Refreshing patch, no functional changes.
Closed by rL350321 (sorry, forgot to mention this review in the commit message).
Rebasing patch so it is no longer dependent on D52298
As suggested my Shiva, this patch has been updated to allow AsmParser::parseExpression to do the work when parsing an identifier.
Updated the patch to address the correctness issues with the udiv and urem patterns. Like with D56264, a dag combine converts an ANY_EXTEND to a SIGN_EXTEND when it operates on an instruction where a 32-bit *W variant could be selected. This is advantageous as it can avoid unnecessary masking/sign-extension of the input operands.
Thu, Dec 20
Thanks, this basically looks good to me. Could you please add some tests for la with invalid operands.
This no longer seems to apply cleanly (needs rebase), but otherwise LGTM. Thanks!
Dec 13 2018
Marking as "planned changes". At least the following patterns are problematic:
This looks good to me, but there's one addition test I'd like to see. You explain that the intended behaviour is that there will be no compile-time error if linker relaxation is enabled. It would be good to have this reflected in a test. Maybe adding extra RUN lines to pcrel-lo12-invalid.s and adding a comment that explains that is expected with linker relaxation enabled.
Forgot to change the status in Phabricator prior to commit, but I did re-review the updated change and it definitely LGTM!
Dec 12 2018
Dec 6 2018
Dec 5 2018
Thanks for sharing this patch series Lewis. As it happens @luismarques has been finishing off his own implementation of TLS support which this unfortunately clashes with. The upside is that we're in a good position to review this work!
Dec 4 2018
@rogfer01: James did actually get a patch for this committed to riscv-asm-manual https://github.com/riscv/riscv-asm-manual/blob/master/riscv-asm.md
Dec 3 2018
Thanks, fixed in rL348117.
Nov 30 2018
I'll update this to include cmpxchg support now D48131 landed.
Just some minor tweaks and I think this is good to go. In addition to the changes mentioned in inline comments, please:
- add nounwind to all the test functions
- Remove tail from the calls to llvm.fma as it doesn't impact the generated code (I have a preference to remove everything that doesn't impact the generated code).
Looks good to me. I double-checked that gas doesn't do this same conversion for fsflags and fsrm. It doesn't, so this seems complete.
LGTM. Needs a trivial tweak to apply. This can land now the RV64I patch was committed. I'll commit now (I believe Lewis doesn't have commit access).
Updated to add note about FPOWI being undefined if integer operand is > 32-bits, as suggested by @efriedma
Submitting some old in-line comments that were accidentally left unsubmitted.
Nov 29 2018
I should probably plan to reflect support for this in my in-flight cmpxchg patch (which adds target-independent support for late lowering of cmpxchg in the same way I added it for atomicrmw). [incidentally - reviews on that patch would be very welcome...]
This patch still applies with minimal fuzz and all tests still pass.
- I'd like to find a solution that doesn't get rid of the helpful error reporting we currently have for load/store. Downgrading all errors to just "invalid operand for instruction" is an unfortunate regression
- The F/D load/store are wrong as this patch has them accepting GPRs rather than FPR32/FPR64. The outs for PseudoSTORE also has a mistake
- We no longer have the shouldForceImmediate logic because associating custom parsers with operands was found to be superior. From some playing with a modified version of this patch it looks like more logic is needed to get these aliases to parse though...
Looks good to me.
Nov 28 2018
I think this is fine, but c.unimp should be proposed on the RISC-V sw-dev mailing list or similar. As we discussed, it seems to make total sense to have this instruction (every other compressed instruction is nameable with c.something). But we should get community feedback first before merging in to LLVM. It's probably also worth highlighting the fact that "unimp" isn't actually guaranteed to trap according to the current RISC-V specs...