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

[Luís Marques via Phabricator via llvm-commits]

luismarques added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122634