In D89025#2324334, @khchen wrote:

RISCV supports -mcpu with default empty arch to align gcc's -mtune behavior since clang didn't support -mtune before. But now clang has -mtune, is it a good idea to remove those options? (ex. rocket-rv32/rv64, sifive-7-rv32/64)

LGTM, but I would like other people to also review this, if possible.
(Just be sure to check/fix the clang-format warnings and the inline comments).

Thanks for the info @kito-cheng

Use __builtin_thread_pointer.

The code makes sense to me and it seems correct, but there may be assumptions about the way that mechanism operates that I'm not familiar with. Some comments:

LGTM. Thanks for the detailed description, with the links.

NFC

RISC-V support for ASan has been merged (see D87582 and its predecessors). This patch should probably be abandoned. Thank you.

RISC-V support for ASan has been merged (see D87582 and its predecessors). This patch should probably be abandoned. Thank you.

Thanks for the fix. Sorry for not catching that during the D86522 review.

In D86278#2319808, @efriedma wrote:

Under what circumstances does this test actually get built? By default, the builtins library doesn't provide these functions, so this will fail to link.

Ping

Ping.

Ping

LGTM. Just remove nop, ensure it isn't actually needed (rerun the tests, etc.), and feel free to commit this patch. Thanks!

For versions 2.29 -- 2.31 you have val = 1172. Is that actually correct? I thought you had earlier obtained a value of 1772 for 2.29, so maybe that's a typo?
Other than that issue, LGTM. Just be sure to verify/adjust that.

In D87577#2301202, @EccoTheDolphin wrote:

By the way - to my surprise, Chris gave me write permissions - So I can commit too.

LGTM. I'll commit this.

I haven't had the opportunity to personally test this, but looking at the code and the information you have provided I think it's probably fine. Please just adjust the code comment.
LGTM.

I can commit this for you. Is assume this the correct author? Alexey Baturo <space.monkey.delivers@gmail.com>

LGTM. Thanks for the patch.

In D62732#2288013, @JDevlieghere wrote:

Sounds reasonable to me. Maybe it's still to early for that but have you tried running (part of) the test suite under QEMU yet? It should give you a pretty good idea of the state of things and gives you a bunch of test coverage for free. You'll probably want to consider setting up a bot for that down the road anyway.

In D62732#2286332, @wallace wrote:

@luismarques , what's the recommended gdb-server implementation you recommend for me to try this on a riscv machine?

In D80465#2283185, @jrtc27 wrote:

Do other architectures with native 64-bit multiplication provide __muldi3? If yes, it should be re-enabled; if no, the test should be fixed. My assumption is it's the latter, but I can see the argument for having an RVM compiler-rt still provide __muldi3 in case linked against code that was compiled without RVM (but, really, that code should just be compiled with RVM).

In D80465#2283180, @pzheng wrote:

I noticed that this patch (along with D86036) caused a compiler-rt test to fail for riscv64 when M extension is enabled.
compiler-rt/test/builtins/Unit/muldi3_test.c:11: undefined reference to `muldi3'
Basically, with this patch, muldi3 is no longer defined anymore for riscv64 when M extension is enabled. Any suggestion on how this should be fixed?

I suppose that an alternative to this implementation might be to tweak the place of use of __NR_futex, like is done in [1]. 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.

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.

• 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?

@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.

In D86198#2243850, @joshua-arch1 wrote:

I have updated my patch and summary. Are there any new updates with the patch review?

I'll leave this to the experts then! :-)

In D86292#2237143, @jrtc27 wrote:

Not so silly; gdb (well, the names are inherited from bfd) has set architecture riscv:rv32/set architecture riscv:rv64 :)

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 :-)

LGTM.

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?

LGTM. (I had already gone down roughly the same path in D86281. This patch additionally adds checks for the alignments of the pointers, so it can supersede D86281)

In D86281#2235413, @arichardson wrote:

Only just saw this after posting my own fix (D86510). This LGTM, and I can rebase my change to enable the 16-byte case on top of this.

In D86036#2233649, @lenary wrote:

I meant muldi3.S and mulsi3.S, not the .inc file - I don't think either is changed by that patch yet?

In D86036#2233630, @lenary wrote:

@thakis this also matches the #ifdefs in the files themselves.

Add riscv32 test.

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.

In D86198#2227975, @joshua-arch1 wrote:

For D84727, I didn't know how to correctly submit an updated patch before. That is why I submitted another patch. For the current stage, you can just abandon D84727.

In D86198#2227534, @joshua-arch1 wrote:

Did you mean D84727? For that patch, no one has given an update feedback so far.

1. You need to provide the full context in the patch. See [1], particularly the part about the -U999999.
2. You need to base your patch on a recent LLVM commit, as this patch doesn't apply cleanly.
3. This patch repeats changes from D86198. Are you abandoning D86198? If so you need to update the status of that review.
4. 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.

In D86043#2222622, @jfb wrote:

Can you add a test that this works, e.g. by having a program which uses these functions, but doesn't link to libatomic?

In D62732#2193729, @simoncook wrote:

As for next steps, if we're happy with the state then I think this should land (assuming qemu is sufficient given it is public), and then we can flesh out other bits which give a better experience. I'm not sure how to connect this to any automated testing, or where to document any way of checking this manually, the state of that isn't quite clear, so any clarity there helps.

In D84833#2184824, @jrtc27 wrote:

I _believe_ we need:

isBranch = 1, isTerminator = 1, isBarrier = 1

?

In D84833#2184534, @jrtc27 wrote:

Other than the TODO, yes (and hopefully you agree with my suggestions here!). I would also like to see branch-relaxation.ll grow RV64 RUN lines for completeness, even if it's probably a bit redundant (and creates a slight explosion..), but I can do that as a follow-up patch.