[PATCH] D63409: [RISCV] Specify various encodings for DWARF exception handling

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai added a comment.

In D63409#1557447 <https://reviews.llvm.org/D63409#1557447>, @edward-jones wrote:

> In D63409#1556886 <https://reviews.llvm.org/D63409#1556886>, @HsiangKai wrote:
>
> > I think D63409 <https://reviews.llvm.org/D63409>, D63411 <https://reviews.llvm.org/D63411>, D63415 <https://reviews.llvm.org/D63415>, and D63416 <https://reviews.llvm.org/D63416> could be merged as one patch. They are all related to exception handling for RISC-V.
>
>
> I kept them as separate changes since they're incremental and I thought it would make things easier to review, but I can squash them into one patch if that is more convenient. At the very least I plan on keeping D63415 <https://reviews.llvm.org/D63415> separate as it includes generic changes.


I found that the test case in this patch will trigger an assertion failed without D63411 <https://reviews.llvm.org/D63411> and there is no test case for D63411 <https://reviews.llvm.org/D63411>. So, I suggest these two patches could be combined at least. It will make review and verification easier.

If you want to keep D63415 <https://reviews.llvm.org/D63415> as a stand alone patch, you should create a test case for it. I think D63415 <https://reviews.llvm.org/D63415> and D63416 <https://reviews.llvm.org/D63416> could be combined. You could think D63416 <https://reviews.llvm.org/D63416> is a show case about how to specify “call site encoding” in architecture dependent part and why you want to do these generic changes. It is only my opinion. For your reference. BTW, your modification looks good.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63409/new/

https://reviews.llvm.org/D63409