[PATCH] D140166: [IR] return nullptr in Instruction::getInsertionPointAfterDef for CallBrInst

Tue Dec 27 07:00:27 PST 2022

dblaikie added a comment.

In D140166#4013772 <https://reviews.llvm.org/D140166#4013772>, @MaskRay wrote:

> I agree that `; CHECK-NOT: {{.*}}:` is uncommon and doesn't seem particularly useful in this case.

I read it as "check that there's not another label between the previous checked label and the following instruction" - which seems OK to me? I might be inclined to firm it up a bit by adding `{{$}}` at the end to make it more clear that it's checking for a label and not a `:` that might appear anywhere else in a match line. (if it needs to be resilient to autogenerated comments after the label - yeah, more regexing would be OK, `{{( ;.*)?$}}` or whatever is necessary to make that work seems OK to me)

> Actually, I think the test has a fair chance that it may bit rot and for maintainability we probably should a comment about the C++ source and some comments about what the test file test.
>
> CC @dblaikie for the `coro-debug.ll` debate. The test was added from D33614 <https://reviews.llvm.org/D33614>.

I'm not sure I'm following all aspects of the discussion here - one is whether we should include the full C++ source and make the IR match the source, which will make it longer but maybe more explainable/maintainable in some ways (handcrafting a few IR instructions is one thing, but if it's so complicated/long we can't reasonably handcraft/modify it, then maybe it's better to generate it from clang instead). Especially for debug information, we tend to use frontend-generated IR, as simplified as possible. Though this test case might be a different situation - the IR is long, but not unmaintainably so, I think? How long's the frontend-generated IR (after as much IR optimization as is acceptable while still preserving the interesting thing to test?)?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140166