[PATCH] D122634: [RISCV] Do not outline CFI instructions when they are needed in EH

[Wang Pengcheng via Phabricator via llvm-commits]

pcwang-thead added a comment.

In D122634#3451886 <https://reviews.llvm.org/D122634#3451886>, @luismarques wrote:

> In D122634#3447432 <https://reviews.llvm.org/D122634#3447432>, @pcwang-thead wrote:
>
>> These conditions existed since the first version of outlining implementation (D66210 <https://reviews.llvm.org/D66210>), so we won't see any differences of position instructions even if I added some tests in this patch.
>
> It's been a few days since my original comment so I may be misremembering the details but here's my perspective:
>
> - Before your patch we had test coverage that showed the difference between a plain `if (MI.isPosition()) return outliner::InstrType::Illegal;` and the then-current implementation.
> - With this patch we no longer have.
> - With this patch you are asserting that we want something more fine-grained than just "don't outline positions". Then let's have test coverage for that more fine-grained condition checking.
>
> Also, in general, I'm in favor of the position that when you touch things you should take the opportunity to address closely related issues, within reason. That's always a tricky balance, and I don't know what the LLVM project policy is for that fine line, but it seems like something that we should strive for :)

Thank you for your explanation, I misunderstood what you meant :)

So I added some more fine-grained tests in D123364 <https://reviews.llvm.org/D123364>:

- `machine-outliner-cfi.mir` shows how we treat CFIs if there is no EH.
- `machine-outliner-throw.ll` shows how we treat CFIs if unwinding is needed.
- `machine-outliner-position.mir` shows that position instructions(which are labels) are illegal to outline.

Are there some other things that I should pay attention to?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122634