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

Thu Dec 22 18:13:16 PST 2022

ChuanqiXu added a comment.

> we probably should a comment about the C++ source

Maybe there is not a C++ source for this test file. On the one side, the Coroutines intrinsics in LLVM is intended to be an extension for LLVM, which shouldn't be dependent on the frontend language. I mean, maybe it is not possible to find a C++ program which can generate the same or similar LLVM IR with the test in LLVM Coroutines tests. On the other side, you may be interested that why we don't test directly for the IR generated from a real C++ program. The answer from me is that it'll generate too many codes from a real C++ coroutines program due to the grammar of C++ (coroutines). Previously when I tried to use the IR generated from C++ directly, I found it is too big and complex so that I'll be confusing about the test (or in another word, I feel developers can't be focus on the test in that way).

So as a result, many tests in LLVM coroutines are written by hand instead of using the generated  IR from a real C++ program.

> and some comments about what the test file test.

For this test file, the things it test should be: the debug information for a coroutine function should be remained in its split functions. (Background: a coroutine will be split into pieces). And personally I feel it is already addressed in the top of the file.

> Actually, I think the test has a fair chance that it may bit rot

We indeed lack some test for coroutines. But for the specific test, I feel it is good as far as I know.

================
Comment at: llvm/test/Transforms/Coroutines/coro-debug.ll:196-198
+; CHECK-NEXT: %1 = load i8*, i8** %coro_hdl.reload.addr
+; CHECK-NEXT: call void @free(i8* %1)
 ; CHECK-NEXT: call void @llvm.dbg.declare(metadata i32 %[[CALLBR_RES]]
----------------
nickdesaulniers wrote:
> ChuanqiXu wrote:
> > nickdesaulniers wrote:
> > > MaskRay wrote:
> > > > ChuanqiXu wrote:
> > > > > nickdesaulniers wrote:
> > > > > > ChuanqiXu wrote:
> > > > > > > We don't care about the inserted checks in the test. It should be fine to check the `llvm.dbg.declare` is in the basic block of `DEFAULT_DEST`. So maybe we can check these 2 lines are not empty or we can check there is no new BB declaration before  `llvm.dbg.declare`.
> > > > > > I'm not sure how best to express that to FileCheck.
> > > > > > 
> > > > > > `; CHECK-NEXT-NOT: {{.*}}:`
> > > > > > 
> > > > > > ?
> > > > > I feel it is a good way to check there is no new BB declaration before `llvm.dbg.declare`
> > > > Actually I think `utils/update_test_checks.py` may not be a bad choice for this large test file.
> > > > 
> > > > If it is not time to migrate the test, I think the patch as-is using `; CHECK-NEXT: %1 = load i8*, i8** ; ...` looks good.
> > > > It doesn't appear that there is more maintenance burden than `; CHECK-NEXT-NOT: {{.*}}:`
> > > Curiously, I tried removing existing CHECK lines from this test, then running `./llvm/utils/update_test_checks.py` on this. The result doesn't pass `llvm-lit -vv llvm/test/Transforms/Coroutines/coro-debug.ll` which may be why this test did not use `./llvm/utils/update_test_checks.py` in the first place.
> > Personally I prefer to not use `utils/update_test_checks.py` in coroutine's tests. Since CoroSplit pass will generate many codes and it is pretty hard to read. Currently when I see the coroutine's tests, I can know the pattern of the interesting part easily instead of reading tons of `CHECK` (that's what I feel when I read the test generated by `utils/update_test_checks.py`).
> > 
> > And for this patch, it is true that `; CHECK-NEXT: %1 = load i8*, i8** ; ... ` wouldn't matter a lot. But I still feel better to make the tests focus on the things it want to test.
> I never actually tested that `; CHECK-NEXT-NOT: {{.*}}:` would be viable.  Why I try it:
> 
> > llvm-project/llvm/test/Transforms/Coroutines/coro-debug.ll:196:9: error: unsupported -NOT combo on prefix 'CHECK'
> 
I think we can refactor them into:

```
; CHECK: [[DEFAULT_DEST]]:
; CHECK-NOT: {{.*}}:
; CHECK: call void @llvm.dbg.declare(metadata i32 %[[CALLBR_RES]]
```

Since currently there is no `NEXT` relationships.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140166