Mostly LLVM work.
- User Since
- Nov 2 2018, 7:48 AM (98 w, 2 d)
Fri, Sep 18
Thu, Sep 17
I suppose that an alternative to this implementation might be to tweak the place of use of __NR_futex, like is done in . Arguably that's cleaner. Or, in the place where you're currently checking for riscv32, check for the case where __NR_futex_ isn't defined but __NR_futex_time64 is, and #define the former in terms of the latter.
I think this makes sense, but I'll let the #libc reviewers actually review/approve this.
This seems roughly OK now. Does anyone with libunwind experience want to give it a thumbs up?
#libunwind people, does this LGTY?
I was hoping others would chime in regarding what instructions should be handled in this hook.
Mon, Sep 14
There is already an implementation of GetWriteFlag for RISC-V. It was provided by D75168, and you can find it in lines 2002-2105.
You'll notice that there is already an implementation of __clear_cache for RISC-V in the file you are contributing to, in line 160. That existing implementation directly performs a syscall. Your new implementation calls a (RISC-V-specific) GNU libc function __riscv_flush_icache, which internally either uses a vDSO (if available) or performs the syscall.
Tue, Sep 8
- Add missing return from main.
Ping. I think all of the review issues (originally added in D81348) have been addressed so this is probably ready to land.
Address review concerns.
There are still issues with compiling and running the sanitizer tests for the added architecture. I think fixing those should be a precondition for this to be considered mergeable.
Have you added the flag "-DLLVM_DEFAULT_TARGET_TRIPLE="riscv64-unknown-linux-gnu"" and "-fsanitize=address" when compiling the sanitizer test?
Thu, Sep 3
@labath @jrtc27 @clayborg Now that we have at least 3 open-source debug servers that we can use to test this with (OpenOCD, QEMU gdbstub, gdbserver) perhaps this can be merged? I had very good results using this patch with OpenOCD. This patch doesn't include automated tests, but I'm not sure what tests would be required for this patch, or that it makes sense to require them at this point. I'll be doing more work for LLDB RISC-V support, and I'll provide tests for specific fixes going forward.
- Fix list of callee saved registers;
- Fix typo;
- Fix trivial clang-format issue.
Commandeering the patch, as discussed in the last RISC-V sync-up call. I'll update it with only minor changes; further development can be done in follow-up patches.
Wed, Sep 2
Fri, Aug 28
Thu, Aug 27
I'll leave this to the experts then! :-)
Tue, Aug 25
This is technically correct, but looking at the other targets it seems that the hook is generally being used more restrictively for instructions where you actually have some expectation of it being a copy/move. Do we really want to go down this route? In principle we'd have to later expand this to e.g. all of the bitmanip instructions that can implement copies and so on, which seems a bit like a reductio ad absurdum. Having tests showing at least a little bit of benefit would help make the case for this patch :-)
Looking at the AArch64 implementation, it seems there is precedent for this hook already including other instructions that are cheap but not necessarily recognized as actual moves by the microarchitecture (i.e. renames). So I guess they may not be, strictly speaking, as cheap as a move, but they are close enough. And that may very well be the point of the hook, otherwise they would just be non-canonical moves?
Mon, Aug 24
Aug 21 2020
Add riscv32 test.
Aug 20 2020
This review is superseded by D86281, due to issues with subscribing llvm-commits.
This is superseded by D86278, due to issues with subscribing llvm-commits.
Aug 19 2020
- You need to provide the full context in the patch. See , particularly the part about the -U999999.
- You need to base your patch on a recent LLVM commit, as this patch doesn't apply cleanly.
- This patch repeats changes from D86198. Are you abandoning D86198? If so you need to update the status of that review.
- I would expect this review to provide a more detailed summary, including the support status of the various sanitizers for RISC-V, and why it's reasonable to enable them.
Aug 18 2020
Aug 17 2020
Aug 16 2020
Aug 4 2020
Jul 30 2020
Jul 29 2020
Jul 6 2020
Alright, let's get this landed! Please just add comments in the expansion functions, noting the need to update these values if the expansion templates ever change, as Lewis says.
LGTM. Good catch Roger!
(I have verified that the code change makes sense based both on tablegen definitions and the sanity test that Roger indicated on the comment).
I'm not overly concerned about the occasional code size increases from doing the optimization for RV32IM, so the loosening of the condition is OK IMO.
Everything else seems to be in order now.
Maybe wait a couple of days more for @lenary's OK.
Jul 4 2020
For these kinds of patches where you add new tests which show a difference in code quality it's helpful if you can split the tests into a separate patch. You can then add that other patch as a dependency for this one, creating a Stack in Phabricator. That way it's easier to see the differences in the generated code.
Jul 2 2020
The patch seems correct to me. I'm not quite sure this is the best place to do the this transformation (guidance on that from other people would be welcome), but I don't have a better solution to propose right now. I'm going to approve this patch, but I suggest you wait a couple more days to give other people an opportunity to comment on that issue.
Regarding the tests, as a nitpicking issue, I would only suggest giving the test functions slightly more semantic names (maybe something like negative_bound_reject, etc.).
Jul 1 2020
This is looking quite good. As Sam says, not splitting BBs is preferable, so while the D79635 issues could be resolved in other ways, if we can make it work like this then that's even better.
I suppose that this patch could also remove everything related to LabelMustBeEmitted.
If anyone wants to provide about the status of setPreInstrSymbol that would be helpful. Currently it's marked as FIXME: This is not fully implemented yet., although Sam indicates that it's being used by x86, so that may be just a holdover.
Jun 29 2020
Jun 24 2020
Use getPointerAlignment instead.
Handle removal of GlobalValue::getAlignment.
Jun 23 2020
Jun 17 2020
Some final tweaks to the unaligned tests. I changed my mind about this when I was about to mark some review comments as done. Sorry about the churn!
Add more consts and alignments.
Jun 15 2020
Jun 14 2020
Address review concerns. Various minor fixes and improvements.
Jun 9 2020
Correctly gate all instances of 128-bit variables and tests.
Add comment about not resetting the test variables between the tests of different operations in the section that tests __atomic_fetch_<op>_* stuff.